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

add task solution #3412

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

Conversation

NataliaDavida
Copy link

@NataliaDavida NataliaDavida commented Jul 29, 2023

Copy link

@denys-danyliuk denys-danyliuk left a comment

Choose a reason for hiding this comment

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

add demo link and test report link to the description of a pr

src/index.html Outdated Show resolved Hide resolved
src/style.css Outdated

.stars {
display: flex;
width: 96px;

Choose a reason for hiding this comment

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

3'rd point in checklist

Choose a reason for hiding this comment

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

isn't fixed. No needs to hardcode width

Copy link

@Viktor-Kost Viktor-Kost left a comment

Choose a reason for hiding this comment

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

Hello!)
Also, please check this comment since it wasn't fixed
#3412 (comment)

If you need any help feel free to ask at fe_chat

src/style.css Outdated
height: 16px;
background-repeat: no-repeat;
background-position: center;
color: #e5e5e5;

Choose a reason for hiding this comment

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

redundant

Suggested change
color: #e5e5e5;

src/style.css Outdated
Comment on lines 11 to 15
background-image: url("./images/star.svg");
width: 16px;
height: 16px;
background-repeat: no-repeat;
background-position: center;

Choose a reason for hiding this comment

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

apply background shorthand

src/style.css Outdated
Comment on lines 19 to 47
.stars__star:not(:last-child) {
margin-right: 4px;
}

.stars--5 .stars__star {
background-image: url("./images/star-active.svg");
}

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

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

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

.stars--4 .stars__star:nth-child(1),
.stars--4 .stars__star:nth-child(2),
.stars--4 .stars__star:nth-child(3),
.stars--4 .stars__star:nth-child(4) {
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.

this type of adding styles isn't flexible. Imagine if you have a case with 10 stars or even more?

  1. use :nth-child(-n + {{here_will_be_a_number_from_1_to_5}})
    Example:
    .my__star:nth-child(-n + 4)
  2. if your selectors have the same style use them as a group
    Example:
.my__selector1, 
.my__selector2 {
  color: #ff0000;
}

src/style.css Outdated

.stars {
display: flex;
width: 96px;

Choose a reason for hiding this comment

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

isn't fixed. No needs to hardcode width

Copy link

@Esceype Esceype left a comment

Choose a reason for hiding this comment

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

Great job!!! Left just one moment

src/style.css Outdated
Comment on lines 19 to 21
.stars--5 .stars__star {
background-image: url("./images/star-active.svg");
}
Copy link

Choose a reason for hiding this comment

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

Move it down as other selectors for modifiers

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

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.

5 participants