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

Stars solution #4955

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

Stars solution #4955

wants to merge 3 commits into from

Conversation

schyrva
Copy link

@schyrva schyrva commented Oct 15, 2024

  • 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.

Hey there! You've done a great job overall, but there are some issues that need to be addressed. There's an extra body tag in your HTML document and it's important to remember that there should only be one body tag in a HTML document. Also, consider using semantic tags more effectively and avoid using the * selector as it impacts performance. 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
/>
</head>
<body>
<h1>Stars</h1>
<body>

Choose a reason for hiding this comment

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

You have a body tag inside another body tag. This is not valid HTML. There should only be one body tag in a HTML document. Please remove the extra body tag.

<div class="stars__star"></div>
<div class="stars__star"></div>
</section>
</body>
</body>

Choose a reason for hiding this comment

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

Here is the closing tag for the extra body tag. Please remove it.

src/index.html Outdated
Comment on lines 21 to 67
<section 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>
</section>

<section 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>
</section>

<section 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>
</section>

<section 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>
</section>

<section 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>
</section>

<section 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>
</section>

Choose a reason for hiding this comment

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

It's great that you're using semantic tags such as section, but the content inside these sections are not semantically clear. Consider adding some descriptive text or alternative content to give more context about these sections. If the div elements are meant to be visual representations, consider using span with appropriate classes or ids.

Comment on lines 1 to 7
body {
margin: 0;
padding: 0;
border: 0;
font-family: Arial, sans-serif;
box-sizing: border-box;
}

Choose a reason for hiding this comment

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

Avoid using the * selector as it impacts performance. It's better to set styles only for elements that require them.

margin: 0;
padding: 0;
border: 0;
font-family: Arial, sans-serif;

Choose a reason for hiding this comment

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

Remember to use fallback fonts - alternative font-family in case the main one doesn't work.

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.

Good job on the HTML structure and use of meaningful class names. There are a few points to improve - consider using a CSS preprocessor for more efficient code and avoid using the '*' selector for better performance. Keep learning and improving! 🚀

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

padding: 0;
border: 0;
font-family: Arial, Helvetica, sans-serif;
box-sizing: border-box;

Choose a reason for hiding this comment

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

It's recommended not to use '*' selector as it impacts performance. It would be better to set styles only for elements that require them.

Comment on lines +16 to +20
.stars--1 .stars__star:nth-child(-n + 1)::before,
.stars--2 .stars__star:nth-child(-n + 2)::before,
.stars--3 .stars__star:nth-child(-n + 3)::before,
.stars--4 .stars__star:nth-child(-n + 4)::before,
.stars--5 .stars__star:nth-child(-n + 5)::before {

Choose a reason for hiding this comment

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

This part of the code is a bit repetitive. You could use a CSS preprocessor like SASS or LESS to loop through these classes and generate them dynamically. This would make your code DRY (Don't Repeat Yourself), easier to maintain and less prone to errors.

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.

2 participants