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

create stars layout #4889

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

Conversation

yzhyhaliuk
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 ❗️

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! All the main functionalities seem to be in place and working correctly. 👍 Just remember to keep an eye on the details like fallback fonts and consistent indentation in the future to make your code even better. 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 Outdated
Comment on lines 15 to 59
<body class="page">
<div class="container">
<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>
<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>
<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>
<div class="stars stars--3">
<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>
<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>
<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>
</div>

Choose a reason for hiding this comment

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

The indentation within the <body> element seems to be off by one space. Ensure that child elements have a 2-space indentation from their parent elements to maintain consistency and readability.

src/index.html Outdated
Comment on lines 16 to 58
<div class="container">
<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>
<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>
<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>
<div class="stars stars--3">
<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>
<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>
<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>

Choose a reason for hiding this comment

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

Add empty lines between sibling blocks of HTML to add some 'air' and simplify reading. For instance, there should be an empty line between the closing </div> of one .stars block and the opening <div> of the next one.

@@ -1 +1,42 @@
/* add styles here */
.page {
margin: 0;

Choose a reason for hiding this comment

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

Avoid using the margin property to zero out the margin on the body element. Instead, set the margin property on the body tag directly in your CSS to ensure consistency and to avoid potential issues with global styles.

Comment on lines +15 to +22
.stars__star {
background-image: url(images/star.svg);
background-repeat: no-repeat;
width: 16px;
height: 16px;
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.

Consider adding a fallback font in case the main one doesn't load. For example, if you're using a web font for the .stars__star, you can specify a generic font family as a backup.

src/style.css Outdated

.stars {
display: flex;
width: fit-content;

Choose a reason for hiding this comment

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

It's a good practice not to fix container sizes unless required. Here, the width is set to fit-content, which is good as it allows the content to dictate the size of the element. Just ensure this is the desired behavior in your layout.

Copy link

@volodymyr-soltys97 volodymyr-soltys97 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 👍
Let's improve your code
Remove the margin right for the last stars
image

src/index.html Outdated
<body>
<h1>Stars</h1>
<body class="page">
<div class="container">

Choose a reason for hiding this comment

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

Redundant wrapper

Suggested change
<div class="container">

src/index.html Outdated
Comment on lines 23 to 24
</div>
<div class="stars stars--1">

Choose a reason for hiding this comment

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

Add the blank line between the blocks

Suggested change
</div>
<div class="stars stars--1">
</div>
<div class="stars stars--1">

src/style.css Outdated

.stars {
display: flex;
width: fit-content;

Choose a reason for hiding this comment

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

Redundant

Suggested change
width: fit-content;

src/style.css Outdated
Comment on lines 24 to 42
.stars.stars--1 .stars__star:nth-child(1) {
background-image: url(images/star-active.svg);
}

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

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

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

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

These selectors must be separated by commas

selector1,
selector2,
...... {
  // ....
}

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

.stars.stars--1 .stars__star:nth-child(1) {

Choose a reason for hiding this comment

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

Fix it everywhere

Suggested change
.stars.stars--1 .stars__star:nth-child(1) {
.stars--1 .stars__star:nth-child(1) {

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! Consider using commas to group selectors

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

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

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

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

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

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

.stars--1 .stars__star:nth-child(1) {

Choose a reason for hiding this comment

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

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

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.

Awesome!

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.

4 participants