Design comparison
Solution retrospective
My first time using JS in a project
Community feedback
- @elaineleungPosted about 2 years ago
Hi Noah, first off, I think you did a good job putting this together. I just got a couple of quick comments after checking out the site:
-
I see that you're using
:focus
for the button that got selected. The problem with this is, if I accidentally click on something else that is not a button (such as background text), the style of the selected button is no longer there, and it looks like no button is selected. I don't suggest using the focus style because of the problem I mentioned and also because the focus style is not meant to be used this way. What I would do instead is to use JS to add or remove a "selected" style class. -
For the star icon, you can try using
aria-hidden
to hide it from screen readers since it's just an icon and really doesn't have much meaning to convey. -
I see that you've used the
onclick
attribute in the HTML for the buttons. While there's nothing really wrong with this, I do suggest keeping all the JS in the script, and also, it's more common now to use event listeners instead of onclick handlers when writing in plain (vanilla) JS.
That's all the thoughts I have, great work over all!
Marked as helpful1 -
- @ericwinkPosted about 2 years ago
Great job on this challenge, Noah! Nicely organized code and it works well. The only feedback I have is you could consider reducing the amount of JS by refactoring to a single function that handles any button press by accepting a parameter:
function selection(input) { selection = input }
Each onClick can call the function with its specific argument:
<li><button class="rating-btn" onclick="selection(1)">1</button></li> <li><button class="rating-btn" onclick="selection(2)">2</button></li>
etc...
Awesome job for a first JS project! Keep up the great work!
Marked as helpful1 - @faha1999Posted about 2 years ago
HI @noahjrgns
Nice job on completing the challenge
you can add the following to center the card vertically and horizontally on the page.
body { min-height: 100vh; width: 100%; margin: 0; }
remove the
margin-top: 10%;
it cuts off.Hope this is helpful.
Marked as helpful0 - @correlucasPosted about 2 years ago
👾Hello Noah, Congratulations on completing this challenge!
I saw your solution preview site and I think it's already really good. Here’s some tips for you to improve it:
Remove the
min-width
and let only themax-width
to make it responsive:main { background-color: var(--dark-blue); padding: 1.75rem; border-radius: 1rem; max-width: 350px; /* min-width: 300px; */ }
To improve the card overall responsibility, you can start to add
flex-wrap
inside the class that manage the section for therating numbers button
and make the adjust to fit in different rows while the container scales down, not that without this property the container doesn't shrink. Here's the code applying these changes:ul { list-style: none; display: flex; flex-direction: row; justify-content: space-between; margin: 1.5rem 0 2rem 0; flex-wrap: wrap; }
You need to include the title for you PAGE. Do that inserting in the <head> the tag <title> →
<title>Rating - Front End Mentor</title>
✌️ I hope this helps you and happy coding!
0
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord