Intro component with signup form

Community feedback

• 

  Ken•935

  @kenreibmanPosted almost 3 years ago

  I forgot to add on... for your JavaScript, instead of changing the innerHTML, I would create a class that reveals the error text like, .show-error with display properties, and add the show-error class to the element with .classList.add('show-error')

  changing the innerHTML is not the most correct way to do it, as well as setting the opacity to 1 and 0 does not hide it for people with screen readers so they will still get an error message regardless of if you can see it on the screen or not.

  You can take a look at my code here as well.

  Marked as helpful

  0

  ---

  

  Misiu•50

  @misiucodesPosted almost 3 years ago

  @kenreibman Ken, this was AMAZING feedback! Thank you so much for taking the time to provide a detailed response on things I could change to make my code better. I learned so much just from this feedback and from looking at your code. Sending so much love from a newbie developer!!

  1
• 

  Ken•935

  @kenreibmanPosted almost 3 years ago

  Hey @misiucodes! Nice job! Since you asked a few questions in your description, I'll give you some feedback.

  could I have done anything different to make the CSS file cleaner?

  • People may disagree, but I like to name all of my HTML elements, including heading, p, etc. since people looking over your code may not know exactly what p you are referring to, when there are multiple paragraph elements in the document.

  • is this optimized for mobile and different screen sizes?*

  • Your media query breakpoint is way too late. I would switch from the desktop to tablet/mobile view at ~1000px for this project. A tablet user would have a hard time seeing their input as the form field becomes too narrow in width. That leads me to the next one,
  • Set a max-width in rem units for your main container, wrapper in your case. It helps with responsiveness, and you can always set a new max-width for your container in a media query.

  Also look at your reports and try to fix some of your accessibility issues as well.

  • The icon-error just needs an aria-hidden= "true"
  • Also using correct HTML5 semantics, you should always wrap your content in a main tag. Your case you could just change your wrapper div into a main class="wrapper".

  I hope this helps!

  Marked as helpful

  0
• 

  T

  Grace•31,730

  @grace-snowPosted almost 3 years ago

  Hi

  You have a few html issues with this that need addressing

  • text should never be in divs alone, use a meaningful element like a paragraph
  • errors need linking to their input with aria-describedby
  • errors need an aria-live attribute
  • all inputs need labelling

  I don't think you need as many structural divs on this either (remember you can put a class on form for example)

  Make sure you Indent your code consistently. It's very easy for bugs to Creep in with poor formatting

  Good luck

  1

  ---

  

  Misiu•50

  @misiucodesPosted almost 3 years ago

  @grace-snow Thank you for taking the time to provide feedback!

  0