intro-component-with-signup-form using Sass and Javascript

Community feedback

• 

  

  Grace•32,130

  @grace-snow

  Posted almost 4 years ago

  Hi

  I’m afraid this doesn’t look right for me viewing on mobile - I’ll add some screenshots to slack for you.

  The content is being cut off on mobile landscape and error messages appear overlapping the input border.

  1. The first issue is because of height 100vh and width 100vw on the body. You never want to restrict height on containers that hold content like that - use min-height where needed instead. I also advise against ever using width 100vw. That is unnecessary as block elements like body are 100% wide by default, and can cause problems because viewport sizes don’t account for things like scrollbars and device notches - including 100vw can actually introduce a horizontal scroll bar
  2. The error message overlap may actually be solved when you fix no1, I’m not sure. I’d need to inspect in browser

  Other suggestions

  • you are heavily nesting css selectors. That’s definitely not advisable and a common mistake when people first start using scss. I recommend only nesting pseudos and media queries so your resultant css stays nice and flat (low specificity)
  • in html are the inputs missing labels?
  • error message elements each need an id and aria-live attribute
  • inputs each need aria-describedby linking to the error id
  • did you consider adding the error icons as background images or pseudo elements? That would be better as they don’t need to clutter up the html. If leaving in html, add aria-hidden or role presentation to those images to make 100% certain they are not announced by screenreaders (apple voiceover will still announce these even though it shouldn’t)
  • never have any text in a div or span alone - use a meaningful element. If in doubt, it’s probably a paragraph
  • use button element not the old input type button
  • if capitalising text, do it in css not the html. Some screenreaders will spell out capitalised text by the letter rather than reading the words
  • if using section element, the section needs a title, preferably with an id. The section element should really then have an aria-labelledby linking to the heading
  • back to css: rather than min width width and margin on the main, why not just have a little padding on the body? It seems over complicated and by adding margin and min width you are potentially conflicting on some mobiles
  • never have any font size declarations in px, that’s important. Convert to rem
  • I know it’s in effort to honour the design, but 10px equivalent (for error messages) is unreadably small and wouldn’t pass accessibility standards. I would challenge that design and have it larger
  • it’s extremely important to have visible focus styles on all interactive elements for users on keyboard
  • letter spacing should be unitless or ems so it always scales with the REM font
  • there appears to be duplication in the media query on this, not sure why (if there is)
  • 376px seems very early to be changing to desktop layout. Is there room for content to be side by side at that point? If not, the media query should be starting at a larger size

  I hope this is helpful for you

  43

  Marked as helpful
• 

  Marlon Passos•920

  @MarlonPassos-git

  Posted almost 4 years ago

  To try to complement what Grace said,

  • Since you used SASS, you could have taken more advantage of the tool to help you, for example: Instead of typing this

  .header {
    margin-bottom: 4rem;
  
    .header-title {
      color: white;
      font-size: 1.7rem;
      line-height: 1.3;
      margin-bottom: 1.2rem;
    }
  

  you could have typed this

  .header {
    margin-bottom: 4rem;
  
    &-title {
      color: white;
      font-size: 1.7rem;
      line-height: 1.3;
      margin-bottom: 1.2rem;
    }
  
  

  Instead of putting the media queried down there separate from the code, you could have done something like this:

  .section {
  
  .. your code..
  
      @media screen and (min-width: 376px) {
          
          .. your mobile code ..
      }
  }
  

  I particularly prefer it this way because the whole style of my component is together and in the end SASS will leave everything together.

  You happen to repeat some code snippets in different elements, you could put them in an @mixin or in variables, for example the leftover buttons

  I like to put the variables, functions and mixins in another separate file, for me it's more organized

  01

  Marked as helpful