The Javascript is working but I'm not satisfied with the code, is there any way to optimize it ? I tried using a for loop to highlight the selected rating and deselect the others, but I couldn't get it to work.
Saulius K.
@TH3RIVAll comments
- @thomaspaysacSubmitted almost 2 years ago@TH3RIVPosted almost 2 years ago
Hi, @stagnant-sys!
There's no need to use JS for highlighting a selected option. You can use radio buttons instead of divs with the same name so only one can be selected at a time.
Then with CSS you hide them and style the labels, so whenever you select a label it selects the button and if you pick a different one, it deselects it.
JavaScript is only needed to switch between Select and Thanks screens (
eventListener
) and to show the selected value (for
loop).You can look at my solution for a better idea on what I mean.
Hope this helps!
Marked as helpful1 - @doyin156Submitted almost 2 years ago
Please feel free to point out any errors or suggestions on things I could have done better. I would love for you to share with me things you learned through years of experience that could make me better at writing codes. Please feel free to drop any resources you think could be helpful. Thanks.
@TH3RIVPosted almost 2 years agoHi, @doyin156!
Your project is not far from being finished, here's a few suggestions to make it even better:
- Your page should contain landmarks, so it makes your code easier to read on what content is what and also helps screen-readers to figure out what is what. In this case your card should be wrapped withing
main
tags and your attribution should be wrapped withinfooter
tags. - You are writing your CSS within the HTML, I would suggest making a separate file for CSS. Imagine this was a big website with a lot of styling and markup.
- Your body is the wrong colour. You are using the text colour. Background should be
Light gray: hsl(212, 45%, 89%)
. - You are using a
h3
without ah1
. Headings are supposed to follow the hierarchy (h1 > h2 > h3 > etc...). In this case it should be ah1
element. You can adjust the font size with CSS. - Try to avoid using
margin
to center your objects. For centering your card I would suggest looking up "Flexbox". - Try to avoid using
px
values as much as possible and userem
instead.
Hope this helps!
Marked as helpful0 - Your page should contain landmarks, so it makes your code easier to read on what content is what and also helps screen-readers to figure out what is what. In this case your card should be wrapped withing
- @Lubka024Submitted almost 2 years ago@TH3RIVPosted almost 2 years ago
Hi, @Lubka024!
Your card looks great!
There's no need or is almost impossible to get it 100% accurate without the actual design files that give you all the margins and paddings etc, so don't worry about it as long as it looks close. Now onto a few suggestions for future improvement:
- Your card should be wrapped within
main
tags, so it is easier to read the code and screen readers know what is the main content. Landmarks are important. - Try to avoid using
px
values and userem
instead. - Try to start using custom CSS properties. While it is not essential here, it is a good habit to develop.
Hope this helps!
Marked as helpful1 - Your card should be wrapped within
- @CarolinaVrlSubmitted almost 2 years ago
All Feedback is welcome, thank you.
@TH3RIVPosted almost 2 years agoHi, @CarolinaVrl!
Your card looks great! There's only a few suggestions I can make for future improvement:
- Your attribution should be either wrapped within
main
tags orfooter
tags. That way screen readers will have easier time figuring out what it is. - Try to avoid using
px
values and userem
instead. - There's no need to declare a fixed with or height for the containers,
max-width
for the card is enough. The rest can be adjusted with margins or padding on the parent.
Keep up the good work! Hope this helps!
Marked as helpful1 - Your attribution should be either wrapped within
- @iulias17Submitted almost 2 years ago
I struggled a little with the responsiveness of the page. If I shrink the page too much, the position of the thumbnail is not centered anymore. Any tips on that? Feedback is highly encouraged and appreciated. Thanks :) !
@TH3RIVPosted almost 2 years agoHi, @iulias17!
I believe it has something to do with
position
property on your<div class="avatar">
. Why not keep in the middle of the card and then center the image within the div. Can't think of any other reason for that behaviour.Hope it helps!
0 - @NunamniroiSubmitted almost 2 years ago
- I'm not sure about my responsive design, how correct it is.
- not sure about naming css selector.
In general, I'm not sure that I used the best practices when creating the site, and I don't know what could have been done better. I will be grateful for the feedback
@TH3RIVPosted almost 2 years agoHi, @Nunamniroi!
Your card is nearly done, it could use a few adjustments:
- You should be using landmarks. This case your card is your main content, so it should be wrapped between
main
tags. Makes it easier to understand the code and helps screen-readers to figure out your main content. - There is no need to use
header
element as it is part of your card. Just wrap your whole card in a container element:
<div class="card"> <div class="first-section"> Section code... </div> <div class="second-section"> Section code... </div> <div class="third-section"> Section code... </div> </div>
- Your button does not need a
<p>
element, just use text of whatever you want it to say. - You should not limit the width of your
body
, you can always limit the size of your elements within thebody
. - You can limit the width of your card using
max-width
property and you can center your card within the page using flexbox on your body element. - To round the card corners you use
border-radius
property. And if it's not showing, you useoverflow: hidden
. - Try to avoid using
px
values and userem
values instead. - Try using custom CSS properties for your projects, it is great habit to develop.
Hope this helps! You got this!
Marked as helpful1 - @ahmedd-osamaSubmitted almost 2 years ago@TH3RIVPosted almost 2 years ago
Hi, @ahmedd-osama!
Your work looks great, but I have a few suggestions to make it even better:
- Your page should contain landmarks, so it makes your code easier to read on what content is what and also helps screen-readers to figure out what is what. In this case your card should be wrapped withing
main
tags and your attribution should be wrapped withinfooter
tags. - Try to avoid using
px
values as much as possible and userem
instead. - Try to start using "custom CSS properties". While this is not essential in this project, it is a good habit to develop.
Hope this helps!
Marked as helpful1 - Your page should contain landmarks, so it makes your code easier to read on what content is what and also helps screen-readers to figure out what is what. In this case your card should be wrapped withing
- @sohailmahmoud17Submitted almost 2 years ago
All comments are welcome. If you have a better solution, let me know, please.. thanks
@TH3RIVPosted almost 2 years agoHi, @sohailmahmoud17!
Your project looks great! I even took some inspiration from this to work on my
background-image
positioning. Thanks! Now onto suggesting to make it even better, you can add JS to it to make it more fun! You can check out my solution to see it in action.Keep up the good work!
0 - @cruz-jerwin15Submitted almost 2 years ago
Good day. I am more likely a backend developer. I take on the challenge because I want to be a full-stack developer. Please give me any tips on how to start being a front-end developer. Your comments are highly appreciated. Thank you in advance.
@TH3RIVPosted almost 2 years agoHi, @cruz-jerwin15!
To answer your question on how to start being a Front-End developer, it all starts with educating yourself. I would suggest a course on "Udemy" made by Colt Steele or taking on the steps from "FreeCodeCamp" or both. That is what i am currently doing. Also helps to learn to do challenges here. I believe that you learn the most by doing it and making mistakes.
Now onto your project and my suggestions:
- Your page should contain landmarks, so it makes your code easier to read on what content is what and also helps screen-readers to figure out what is what. In this case your card should be wrapped withing
main
tags and your attribution should be wrapped withinfooter
tags. - Try to avoid using
px
values as much as possible and userem
instead. - Try to start using "custom CSS properties". While this is not essential in this project, it is a good habit to develop.
Hope this helps!
Marked as helpful1 - Your page should contain landmarks, so it makes your code easier to read on what content is what and also helps screen-readers to figure out what is what. In this case your card should be wrapped withing
- @sankalp414Submitted almost 2 years ago
This is my first challenge solution in front end mentor platform .I have made a responsive qr-card component . Any feedback about this will be appreciated
@TH3RIVPosted almost 2 years agoHi, @sankalp414!
Your page looks pretty nice, but I have a few suggestions for you:
- Your page should contain landmarks, so it makes your code easier to read on what content is what and also helps screen-readers to figure out what is what. In this case your card should be wrapped withing
main
tags and your attribution should be wrapped withinfooter
tags. - You are writing your CSS within the HTML, I would suggest making a separate file for CSS. Imagine this was a big website with a lot of styling and markup.
- |For centering your card I would suggest looking up "Flexbox".
- Try to avoid using
px
values as much as possible and userem
instead. - Try to start using "custom CSS properties". While this is not essential in this project, it is a good habit to develop.
Hope this helps!
Marked as helpful0 - Your page should contain landmarks, so it makes your code easier to read on what content is what and also helps screen-readers to figure out what is what. In this case your card should be wrapped withing
- @superuser2345Submitted almost 2 years ago
I have found it difficult to give my divs full page heights. I will be grateful to get a solution to this.
I want to know the basics of Web Designs need to be consider everytime we make one?
@TH3RIVPosted almost 2 years agoHi, @superuser2345!
I have a few suggestions for you:
- Your page should contain landmarks, so it makes your code easier to read on what content is what and also helps screen-readers to figure out what is what. In this case your card should be wrapped withing
main
tags and your attribution should be wrapped withinfooter
tags. - You are writing your CSS within the HTML, I would suggest making a separate file for CSS. Imagine this was a big website with a lot of styling and markup.
- You have separate index files for desktop and mobile, your page should be one file and responsive based on the screen width. I would suggest looking up "Media Queries". Currently your page is not looking good on a mobile screen. Use browser inspect tools to test different screen widths and responsiveness.
- Centering elements using margins is considered bad practice, I would suggest looking up "Flexbox" as an approach to aligning elements.
- Try to avoid using
px
values as much as possible and userem
instead. - Try to start using "custom CSS properties". While this is not essential in this project, it is a good habit to develop.
- I would also suggest using a CSS reset.
Hope this helps!
Marked as helpful0 - Your page should contain landmarks, so it makes your code easier to read on what content is what and also helps screen-readers to figure out what is what. In this case your card should be wrapped withing
- @ralphvirtucioSubmitted almost 2 years ago
I notice that when hovering my Learn More button my whole content is slightly moving up ? Why is that happening? I try googling but I can't find any answer.
@TH3RIVPosted almost 2 years agoHi, @ralphvirtucio!
Your content is not moving up, your whole container is expanding and that is because you are adding
2px
border onbutton:hover
. This then expands your whole container. Move theborder
property from the:hover
state to the button itself and that should fix it.Hope this helps!
Marked as helpful0