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

Community feedback

• 

  T

  Grace•31,230

  @grace-snowPosted about 3 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

  Marked as helpful

  4

  ---

  

  T

  Grace•31,230

  @grace-snowPosted about 3 years ago

  Ah how annoying - I wrote a load of us feedback as well, then my phone died and it deleted it! 🤦‍♀️

  I’ll have to send that over later as I’m starting work now

  Keep on coding

  Marked as helpful

  1

  

  Brian Carlo Birondo•320

  @Briancarlo24Posted about 3 years ago

  @grace-snow

  Wow this is so much effort. I really appreciate this kind of feedback and I assure you I read it all.

  Most of the feedbacks here are very new to me and I have a lot of questions actually but I'm not sure if you have time. However can you show me some code or that is similar to this challenge just so I'll will have something to compare with and to study even?

  Most helpful to me:

  1. Never use vh on height. Use min-height instead.

  2. I really couldn't find a way to properly structure or arrange the elements without wrapping it in div. (I really hope you could show me some sample so that I will have any idea how to do it properly).

  3. To be honest with you I don't know what an 'aria-live' and it's use but this opens it for me to do research. which I appreciate so much.

  4. I wonder why I should never have a div or span alone? Is there some bad effects to the website or the speed maybe? or maybe it's just bad code. Just wanna know out of curiosity.

  5. I didn't know that avoiding in writing in capitalize text will affect the screen reader but this is really helpful.

  6. I wonder why is it important to have an ID for section. is that just the starndard of coding?

  7. Also how to link aria-labelled to the heading? This is also very new to me. I'm not sure what is it's use but I'll try doing a little bit of research on this.

  8. I tried like making the icons as a pseudo elements like this

  input::after { content: "url(../link/here.svg)"; display: block; position: absolute: height: 50px; width: 50px; } but for some reason it just doesn't show up in input. So I just give up and use the img tag.

  9. Never use font-size declaration in px. (I'm keeping this in mind)

  10. I don't quite understand this question but I'll try to answer. It is on the challenge to use 375px for mobile and 1000+px something for desktop. that's why I'm using the 375px or in this case 376px.

  Thank you very much for you feedback. You have no Idea how helpful it is for someone like me who is pretty new to frontend development.

  0

  

  Brian Carlo Birondo•320

  @Briancarlo24Posted about 3 years ago

  Thank you very much @grace-snow, you have no idea how helpful your feedback are for me :D

  I tried to fix the mobile as well as I didn't notice it at first. Thanks for pointing that out!

  Have a blessed day!

  0
• 

  Marlon Passos•940

  @MarlonPassos-gitPosted about 3 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

  Marked as helpful

  0

  ---

  

  Brian Carlo Birondo•320

  @Briancarlo24Posted about 3 years ago

  @MarlonPassos-git Ohh nice. So as long as the name of the parent is the same with the child you can replace it with ''&''. That's how I understand it.

  Also I never thought this is possible until now.

  .section {

  .. your code..

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

  }

  Thanks for this. I will try using this in my future frontendmentor challenge.

  1