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.

Sign up for GitHub

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

Jump to bottom

add solution #4879

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add solution #4879

wants to merge 1 commit into from

Conversation

Vadym-Kit
Copy link

  • DEMO LINK

  • TEST REPORT LINK

  • Yellow stars are added with container modifier + pseudo-selector (NO extra classes)

  • Each BEM block has its own separate file

  • All Typical Mistakes from BEM lesson theory are checked.

  • Code follows all the Code Style Rules ❗️

mateacademy-ai-mentor
mateacademy-ai-mentor approved these changes Oct 1, 2024
View reviewed changes
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor 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 on your Pull Request! 🌟 Just a few minor points to keep in mind for future projects: be mindful of consistent margin usage, maintain proper indentation, and ensure clean structuring with empty lines between sibling blocks. These small details will enhance the readability and maintainability of your code. Keep up the good work!

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

src/index.html
@@ -13,6 +13,51 @@
/>
</head>
<body>
<h1>Stars</h1>
<div class="display">
Copy link

Choose a reason for hiding this comment

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

Use 2 spaces for indentation in your file. The div element on line 16 should be indented with 2 spaces as it is a child of the body element.

src/index.html
@@ -13,6 +13,51 @@
/>
</head>
<body>
<h1>Stars</h1>
<div class="display">
<div>
Copy link

Choose a reason for hiding this comment

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

Remember about correct indentation between parent and child elements. The div element on line 17 should be indented with 2 spaces as it is a child of the div element on line 16.

src/index.html
Comment on lines +18 to +24
<div class="stars stars--0">
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
</div>
Copy link

Choose a reason for hiding this comment

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

Add empty lines between multiline sibling blocks of HTML. There should be an empty line before line 18 and after line 24 to separate the sibling div blocks.

src/index.html
Comment on lines +25 to +31
<div class="stars stars--1">
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
</div>
Copy link

Choose a reason for hiding this comment

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

Add empty lines between multiline sibling blocks of HTML. There should be an empty line before line 25 and after line 31 to separate the sibling div blocks.

src/index.html
Comment on lines +32 to +38
<div class="stars stars--2">
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
</div>
Copy link

Choose a reason for hiding this comment

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

Add empty lines between multiline sibling blocks of HTML. There should be an empty line before line 32 and after line 38 to separate the sibling div blocks.

src/index.html
Comment on lines +46 to +52
<div class="stars stars--4">
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
</div>
Copy link

Choose a reason for hiding this comment

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

Add empty lines between multiline sibling blocks of HTML. There should be an empty line before line 46 and after line 52 to separate the sibling div blocks.

src/index.html
Comment on lines +53 to +59
<div class="stars stars--5">
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
</div>
Copy link

Choose a reason for hiding this comment

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

Add empty lines between multiline sibling blocks of HTML. There should be an empty line before line 53 and after line 59 to separate the sibling div blocks.

src/style.css

.display {
display: flex;
margin: 0 auto;
Copy link

Choose a reason for hiding this comment

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

Be consistent with your margins (Add only top or bottom, but not both). Here you have a margin set to '0 auto', which is fine for horizontally centering, but make sure you're not adding unnecessary vertical margins elsewhere that could lead to margin collapse.

src/style.css
Comment on lines +27 to +28
margin: 2px;
margin-right: 6px;
Copy link

Choose a reason for hiding this comment

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

Avoid setting margins on all sides of an element. It's better to choose either top or bottom for vertical spacing to prevent margin collapse issues. Since you have a right margin defined on line 28, you might want to remove the general margin on line 27, or make sure it's intentional and won't cause any layout issues.

src/style.css

.stars--5 :nth-child(-n + 5) {
background-image: url(images/star-active.svg);
margin-bottom: 0;
Copy link

Choose a reason for hiding this comment

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

You have set 'margin-bottom: 0' on the last .stars--5 child, which is good for overriding the default margin set on .stars__star. However, ensure that this is the only place where vertical margins are being manipulated to maintain consistency.

Copy link

Choose a reason for hiding this comment

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

Consider moving the margin bottom to the :last-child selector

anastasiiavorobiova
anastasiiavorobiova suggested changes Oct 1, 2024
View reviewed changes
Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Great work! Almost done!

src/style.css
margin-right: 6px;
}

.stars--1 :nth-child(1) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
.stars--1 :nth-child(1) {
.stars--1 :nth-child(1),
.stars--2 :nth-child(-n + 2),
... {

Consider using commas to group selectors

src/style.css
Comment on lines +35 to +45
.stars--2 :nth-child(-n + 2) {
background-image: url(images/star-active.svg);
}

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

.stars--4 :nth-child(-n + 4) {
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.

Suggested change
.stars--2 :nth-child(-n + 2) {
background-image: url(images/star-active.svg);
}
.stars--3 :nth-child(-n + 3) {
background-image: url(images/star-active.svg);
}
.stars--4 :nth-child(-n + 4) {
background-image: url(images/star-active.svg);
}

src/style.css

.stars--5 :nth-child(-n + 5) {
background-image: url(images/star-active.svg);
margin-bottom: 0;
Copy link

Choose a reason for hiding this comment

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

Consider moving the margin bottom to the :last-child selector

src/style.css
background-image: url(images/star.svg);
background-size: cover;
margin: 2px;
margin-right: 6px;
Copy link

Choose a reason for hiding this comment

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

Avoid overriding margins in the same selectors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anastasiiavorobiova anastasiiavorobiova anastasiiavorobiova requested changes

@mateacademy-ai-mentor mateacademy-ai-mentor mateacademy-ai-mentor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Vadym-Kit @anastasiiavorobiova @mateacademy-ai-mentor