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

layout stars #4900

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

layout stars #4900

wants to merge 3 commits into from

Conversation

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

Hey there! 👋 It looks like you've done a great job overall, but there are a couple of issues that need your attention before we can move forward. The fixed container size might lead to content overflow, which could affect the layout of your page. Also, there are some formatting inconsistencies that could be improved for better readability. Keep up the good work, and with a few tweaks, your code will be even better! 🚀

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

Comment on lines 10 to 13
<link
rel="stylesheet"
href="./style.css"
href="style.css"
/>

Choose a reason for hiding this comment

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

The attributes of the link tag should also be formatted with each attribute on a new line, following the same indentation rules as the meta viewport tag.

Comment on lines +16 to +63
<div class="stars__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.

Consider adding empty lines between sibling div blocks to improve readability, as recommended in the checklist.

src/style.css Outdated
Comment on lines 6 to 9
.stars__container {
width: 96px;
height: 116px;
border-radius: 0.5px;

Choose a reason for hiding this comment

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

Don't fix container size (if there is no such a requirement). Let the content size dictate it to avoid overflow or accidental scrollbar. If this fixed size is not a specific requirement, consider using min-width and min-height instead or padding to control the size.

.stars__container {
width: 96px;
height: 116px;
border-radius: 0.5px;

Choose a reason for hiding this comment

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

The border-radius value of 0.5px is very small and might not be noticeable. If you intended to create a rounded effect, consider using a larger value.

.stars__star {
width: 16px;
height: 16px;
margin-right: 4px;

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 margins, but not both, to avoid potential margin collapse issues. Here you have a right margin, which is fine, but ensure consistency throughout your layout.

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! 🌟 There are a few minor suggestions to keep in mind for future work, such as ensuring consistency in image URLs and margin usage, letting content size dictate container dimensions, improving code readability with proper spacing, and adhering to best practices for indentation and attribute formatting. These are not critical issues but addressing them will enhance your code quality. Keep it up! 💻✨

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

/>
</head>
<body>
<h1>Stars</h1>
<div class="stars__container">

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 instead of the 4 spaces currently used to align with the HTML formatting best practices.

<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 multiline sibling blocks of HTML to add some 'air' and simplify reading, as per the checklist.

Comment on lines +6 to +14
.stars__container {
min-width: 96px;
min-height: 116px;
border-radius: 0.5px;
position: absolute;
display: flex;
flex-direction: column;
justify-content: space-between;
}

Choose a reason for hiding this comment

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

Avoid fixing the container size like using min-width and min-height unless it is a specific requirement. Let the content size dictate the container size to prevent overflow or accidental scroll bars.

.stars {
display: flex;
justify-content: space-between;
margin-bottom: 0;

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. It's recommended to add only top or bottom margins, but not both, to avoid potential margin collapse issues.

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

The background-image URL in the .stars--* .stars__star selectors should be consistent with the one used in the .stars__star.active selector. Ensure they are the same to avoid loading different images that are meant to be the same.

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