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 solution #5507

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

add solution #5507

wants to merge 2 commits into from

Conversation

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

Hey! 👋 Thanks for your submission! I've reviewed your code, and while the HTML structure and overall code quality look good, there are a few things we need to address before merging.

Specifically, in src/index.html, the indentation should use 2 spaces, and you should add empty lines between multiline sibling blocks of HTML for better readability as per the checklist.

In src/style.css, please ensure all lines are within 80 characters. Also, avoid using the * selector and please, ensure that there are no vertical margins added to the .stars elements, as mentioned in the task description. Keep up the great work! 👍

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

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

🤖 If you have any comments about this review, please leave a message in the Add a comment field below and mention me with @mateacademy-ai-mentor tag. I will read your message when I perform the next review of your pull request.

src/style.css Outdated
Comment on lines 1 to 3
* {
box-sizing: border-box;
}

Choose a reason for hiding this comment

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

The description file and checklist discourage the use of the * selector due to its performance impact. Consider styling specific elements instead.


body {
margin: 0;
display: block;

Choose a reason for hiding this comment

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

The description file mentions not adding vertical margins between blocks. Ensure that you are not adding any vertical margins to the .stars elements.

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! Overall, your code looks great and follows many of the guidelines! 🎉 I'm approving this PR so you can move forward, but please take a look at the following points to improve your code:

  • src/index.html: Please add empty lines between multiline sibling blocks of HTML for better readability. This helps to visually separate different sections of your code.
  • src/style.css: You've done a good job avoiding the * selector and using classes. However, some lines exceed the 80-character limit. For example, the lines starting with .stars--1 .stars__star:nth-child(-n + 1) are too long. You can break long lines into multiple shorter lines to improve readability. Adhering to this limit helps ensure your code is easy to read and maintain across different environments.

Keep up the good work, and remember to address these comments in future PRs! Reviewing these comments will help you improve your code and learn from your mistakes. 👍

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

Comment on lines +24 to +28
.stars--1 .stars__star:nth-child(-n + 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) {

Choose a reason for hiding this comment

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

These lines are longer than 80 characters. Please ensure all lines are within the 80-character limit as per the Code Style Rules ❗️.

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