Remove and add a display class when submitting the form using JS.

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted 9 months ago

  Looks good overall but there is room for improvement:

  1. All content should be contained within landmarks. That means both form panels should be in main, and attribution in footer.
  2. Decorative icons should have empty alt
  3. The radios should be in a fieldset with a legend (or visually-hidden legend). It's like the question is not associated to the answers in any way right now.
  4. In js don't get the selected value from a quest selector! Get it from the form data. The chosen value is already available to you with this.rating.value (ie form.rating.value where "rating" is the name of the field whose value you want)
  5. Consider using the hidden attribute for showing and hiding instead of a class (optional)
  6. The thank you panel should be wrapped in an extra div that has aria-live on it and is always present in the DOM (I.e. Is not display none). This means the thank you content would be announced to assistive tech users when it appears.
  7. Never limit the height of elements that contain text, including the body. Min-height is fine to use, height is not.
  8. Do not set an explicit width on the component. Use max-width instead.
  9. Never set font size in px, not even once
  10. The radios must be inclusively hidden! That means using a visually-hidden class not setting opacity to 0, which can lead to elements being hidden from some screen readers.
  11. This challenge does not need a media query. But if it did (note for future challenges) that media query would need to be min width in a mobile first layout (99% of styling will be mobile first) and would need to be defined in rem or em not px.
  12. Make sure the component has a little margin on all sides or a wrapping element (eg body in this case) has a little padding on all sides, so the component cannot hit screen edges for any users. It can currently hit screen edges on the top and the bottom on mobile and on all sides on larger screens for users with larger text sizes

  Marked as helpful

  0

  ---

  

  Michael Degorre•160

  @MDEGORREPosted 9 months ago

  @grace-snow Hi Grace thank you for your advices i have a question regarding the fieldset: should i include the button inside ? Could not find any appropriate answer.

  0

  

  T

  Grace•31,270

  @grace-snowPosted 9 months ago

  @MDEGORRE no. A fieldset is for a group of inputs. A legend is a title for that group. A legend must be the first child of a fieldset. Mdn docs are very good

  0