@GregLyons
Posted
Looks like everything's working properly, and I like how you make the submission button disabled until the user selects a rating; I don't think that was in the challenge itself, so it's a good catch. I have a few suggestions as well:
1) You should be using semantic HTML (<h1>
-<h6>
tags, <p>
, <button>
) instead of <div>
's whenever applicable. This makes your website easier to understand for screen readers and SEO. One example is that your rating buttons should actually be <button> elements instead of <div>
's. Another benefit of this is that users can then use "Tab" and "Shift + Tab" to go through the ratings with a keyboard. Some people physically have trouble using computer mice and need to use a keyboard, whereas others (like myself sometimes) just prefer to use keyboards sometimes for filling out forms (like tabbing between input fields). This behavior is built into <button>
and <input>
elements and so doesn't require much more work on your end (as long as you remember to do it), but it would be quite hard to code for <div>
elements. Using semantic HTML lets you leverage a lot of built-in stuff to make your websites even better without doing more than choosing the correct element.
2) I believe display: none;
applies not only to the element, but to its children as well. This would help you not have to apply your .non-display
class to elements one-by-one like you do in your JavaScript. For this to work, you could take your .summary
, .thakyou-title
, and .thankyou-para
elements in wrap them in a <div>
, with a class like .thankyou-wrapper
. Then, you could wrap the other elements (your initial component with the rating feature) in another <div>
, with a class like .rating-wrapper
. Then you can just add/remove the .non-display
class on those two wrapper <div>
's.
3) You can read more about display: none;
here. Near the middle of the page, you can see that with display: none;
, your page will be displayed as if the element isn't there. Thus, these rules in your .non-display
class:
margin: none;
border: none;
width: none;
height: none;
aren't necessary/don't do anything, as your element isn't taking up space when it has display: none;
. They would be relevant if you used visibility: hidden;
instead, in which case the element disappears but still takes up space, but in that case you may as well just use display: none;
in the first place.
Hope this helps!
Marked as helpful