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 #4845

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

Conversation

alena-bondar
Copy link

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.

It looks like you've made a great effort on this Pull Request! However, there are a few important adjustments to make before it's ready to merge. Please address the feedback regarding fallback fonts, redundant CSS, the use of the * selector, attribute formatting, and consistent indentation. These changes will ensure your code is more maintainable and adheres to standard practices. Keep up the good work, and I'm sure your code will be even better with these improvements! 😊👍

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

Comment on lines 15 to +62
<body>
<h1>Stars</h1>
<div class="stars stars--0">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
</div>

<div class="stars stars--1">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
</div>

<div class="stars stars--2">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
</div>

<div class="stars stars--3">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
</div>

<div class="stars stars--4">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
</div>

<div class="stars stars--5">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
</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 multiline sibling blocks of HTML to simplify reading the code. However, make sure not to add them between parent and child elements. For example, there should be an empty line between the closing </div> tag of one block and the opening <div> tag of the next block.

Copy link
Author

Choose a reason for hiding this comment

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

I already have an empty line between the closing tag of one block and the opening

tag of the next block.

src/style.css Outdated
Comment on lines 1 to 4
* {
margin: 0;
padding: 0;
box-sizing: border-box;

Choose a reason for hiding this comment

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

Don't use the * selector as it impacts performance. Instead, set styles only for elements that require them.

src/style.css Outdated
box-sizing: border-box;
}

body {

Choose a reason for hiding this comment

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

The body tag is an exception where you can use tag names for styling, so this is acceptable. However, since you have already reset the margin in the * selector, this is redundant and can be removed.

justify-content: flex-start;
}

.stars__star {

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. Here you could specify a fallback font-family for .stars__star if you have a custom font applied.

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 suggestions to enhance your code's readability and maintainability. Remember to use fallback fonts to ensure text is always displayed nicely, keep lines under 80 characters for better readability, and maintain consistent indentation to make the structure of your HTML clear. These aren't blockers but following these practices will 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! ✨

Comment on lines 15 to +62
<body>
<h1>Stars</h1>
<div class="stars stars--0">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
</div>

<div class="stars stars--1">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
</div>

<div class="stars stars--2">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
</div>

<div class="stars stars--3">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
</div>

<div class="stars stars--4">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
</div>

<div class="stars stars--5">
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
<span class="stars__star"></span>
</div>

Choose a reason for hiding this comment

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

The tag and its children should have a 2-space indentation to reflect the nesting level properly.

}

.stars__star {
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. It's a good practice to specify a generic family as a fallback. For example, font-family: Arial, 'Helvetica Neue', Helvetica, sans-serif; ensures that if Arial is not available, the browser will try Helvetica Neue, then Helvetica, and finally any available sans-serif font.

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