-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Develop #5377
base: master
Are you sure you want to change the base?
Develop #5377
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! 🎉 Your implementation meets all the task requirements and constraints perfectly. The HTML and CSS are well-structured, and you've followed the BEM methodology correctly. Keep up the excellent work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start!
- There are a lot of excessive styles
- Consider using classes with modifiers to set the number of active stars
- Try to pass tests
src/style.css
Outdated
display: flex; | ||
justify-content: center; | ||
align-items: center; | ||
height: 100vh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
display: flex; | |
justify-content: center; | |
align-items: center; | |
height: 100vh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's wrong here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant classes which can break tests
src/style.css
Outdated
align-items: center; | ||
justify-content: center; | ||
width: calc(5 * (var(--star-size) + var(--star-spacing))); | ||
height: calc(5 * (var(--star-size) + var(--row-spacing))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
align-items: center; | |
justify-content: center; | |
width: calc(5 * (var(--star-size) + var(--star-spacing))); | |
height: calc(5 * (var(--star-size) + var(--row-spacing))); |
src/style.css
Outdated
opacity: 1; | ||
padding: 0; | ||
margin: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opacity: 1; | |
padding: 0; | |
margin: 0; |
Reference Test Diff label: stars--0selector: .stars--0 filename: stars--0.png diff%: 9.06 diff-x: 0 diff-y: 0 Reference Test Diff label: stars--1selector: .stars--1 filename: stars--1.png diff%: 11.25 diff-x: 0 diff-y: 0 Reference Test Diff label: stars--2selector: .stars--2 filename: stars--2.png diff%: 13.44 diff-x: 0 diff-y: 0 Reference Test Diff label: stars--3selector: .stars--3 filename: stars--3.png diff%: 15.63 diff-x: 0 diff-y: 0 Reference Test Diff label: stars--4selector: .stars--4 filename: stars--4.png diff%: 17.81 diff-x: 0 diff-y: 0 Reference Test Diff label: stars--5selector: .stars--5 filename: stars--5.png diff%: 20.00 diff-x: 0 diff-y: 0 Reference Test Diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, there are a few comments to fix.
src/style.css
Outdated
display: flex; | ||
justify-content: center; | ||
align-items: center; | ||
height: 100vh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not fixed.
background-position: center; | ||
background-image: url('/src/images/star.svg'); /* Default star */ | ||
flex-shrink: 0; | ||
margin-right: var(--star-spacing); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this margin for last-child.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you haven't changed anything. Please feel free to ask for help in fe_chat.
src/style.css
Outdated
display: flex; | ||
justify-content: center; | ||
align-items: center; | ||
height: 100vh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant classes which can break tests
background-position: center; | ||
background-image: url('/src/images/star.svg'); /* Default star */ | ||
flex-shrink: 0; | ||
margin-right: var(--star-spacing); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not fixed
src/index.html
Outdated
<div class="stars stars--5"> | ||
<div class="stars__star active"></div> | ||
<div class="stars__star active"></div> | ||
<div class="stars__star active"></div> | ||
<div class="stars__star active"></div> | ||
<div class="stars__star active"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div class="stars stars--5"> | |
<div class="stars__star active"></div> | |
<div class="stars__star active"></div> | |
<div class="stars__star active"></div> | |
<div class="stars__star active"></div> | |
<div class="stars__star active"></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> |
Checklist
❗️ Replace
<your_account>
with your Github username and copy the links toPull Request
description:❗️ Copy this
Checklist
to thePull Request
description after links, and put- [x]
before each point after you checked it.Typical Mistakes
fromBEM
lesson theory are checked.