Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solution | layout-stars #3757

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Solution | layout-stars #3757

wants to merge 5 commits into from

Conversation

Reyti
Copy link

@Reyti Reyti commented Nov 1, 2023

@@ -21,8 +21,8 @@ Follow

❗️ Replace `<your_account>` with your Github username and copy the links to `Pull Request` description:

- [DEMO LINK](https://<your_account>.github.io/layout_stars/)
- [TEST REPORT LINK](https://<your_account>.github.io/layout_stars/report/html_report/)
- [DEMO LINK](https://Reyti.github.io/layout_stars/)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check your deployment since DEMO link leads to readme.md and TEST link leads to 404 page

src/style.css Outdated
Comment on lines 21 to 43
.stars--0 .stars__star:nth-child(n):not(:nth-child(n+1)) {
background-image: url("images/star-active.svg");
}

.stars--1 .stars__star:nth-child(n):not(:nth-child(n+2)) {
background-image: url("images/star-active.svg");
}

.stars--2 .stars__star:nth-child(n):not(:nth-child(n+3)) {
background-image: url("images/star-active.svg");
}

.stars--3 .stars__star:nth-child(n):not(:nth-child(n+4)) {
background-image: url("images/star-active.svg");
}

.stars--4 .stars__star:nth-child(n):not(:nth-child(n+5)) {
background-image: url("images/star-active.svg");
}

.stars--5 .stars__star:nth-child(n):not(:nth-child(n+6)) {
background-image: url("images/star-active.svg");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use as a list

Suggested change
.stars--0 .stars__star:nth-child(n):not(:nth-child(n+1)) {
background-image: url("images/star-active.svg");
}
.stars--1 .stars__star:nth-child(n):not(:nth-child(n+2)) {
background-image: url("images/star-active.svg");
}
.stars--2 .stars__star:nth-child(n):not(:nth-child(n+3)) {
background-image: url("images/star-active.svg");
}
.stars--3 .stars__star:nth-child(n):not(:nth-child(n+4)) {
background-image: url("images/star-active.svg");
}
.stars--4 .stars__star:nth-child(n):not(:nth-child(n+5)) {
background-image: url("images/star-active.svg");
}
.stars--5 .stars__star:nth-child(n):not(:nth-child(n+6)) {
background-image: url("images/star-active.svg");
}
.stars--0 .stars__star:nth-child(n):not(:nth-child(n+1)),
.stars--1 .stars__star:nth-child(n):not(:nth-child(n+2)),
.stars--2 .stars__star:nth-child(n):not(:nth-child(n+3)),
.stars--3 .stars__star:nth-child(n):not(:nth-child(n+4)),
.stars--4 .stars__star:nth-child(n):not(:nth-child(n+5)),
.stars--5 .stars__star:nth-child(n):not(:nth-child(n+6)) {
background-image: url("images/star-active.svg");
}

src/style.css Outdated
@@ -0,0 +1,43 @@
* {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not a good practice to reset styles using * selector

src/style.css Outdated
}

.stars__star {
margin: 0 2px;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use :not(:last-child) to set margin-right to each star besides last one

src/style.css Outdated

.stars__star {
margin: 0 2px;
margin-top: 2px;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant

Suggested change
margin-top: 2px;

src/style.css Outdated
Comment on lines 14 to 15
width: 16px;
height: 14px;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should have the same width and height

@Reyti
Copy link
Author

Reyti commented Nov 1, 2023

  1. Replaced ul, li to div elements, and adapted styles to them.
  2. Fixed issue related to DEMO LINK.
  3. Replaced * with body to reset styles.

Copy link

@andriy-shymkiv andriy-shymkiv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good job. left one comment. approved in advance

background-image: url("images/star.svg");
background-repeat: no-repeat;
background-position: center;
margin-right: 4px;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove margin-right for last child

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants