HTML5 + SCSS Level up! Clean and efficient code!

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, really awesome work as well on this one. Desktop layout is really great, it is responsive and the mobile state looks really great as well.

  Some suggestions would be:

  • For a fluid layout, I always use this function that I created in scss:

  # I separate this module 
  @use "sass:math";
  
  @function rem($px) {
    $value: math.div($px, 16) + rem;
    @return $value;
  }
  
  @function fluid($min, $pref, $max) {
    $size: clamp(#{rem($min)}, #{$pref}, #{rem($max)});
    @return $size;
  }
  

  And on any properties that requires number value, I could just use like: link to my scss structuring

  # 
  @use "_functions.scss" as func;
  
  font-size: func.fluid(16, 1.4vw, 18;
  padding: func.fluid(32, 7vw, 56) func.fluid(48, 17vw, 80);
  margin: ...
  max-height:...
  

  Though you will need to use the dev tools a lot to get the proper preferred size, the middle one value. I kept it as close to the original clamp.

  • Website-logo- link a tag should either use aria-label or screen-reader only text inside it, that defines where this link would take the user. Since typically a website-logo links to homepage, use "homepage" as the value for what ever method you will use.
  • On your logo-svg, you should do it this way for the title to work properly.

  <svg aria-describedBy="logosvg">
    <title id="logosvg"> fylo </title>
    the rest of the svg code
  </svg>
  

  Though I prefer using svg as the src value for the img so that I will just have to use alt.

  • Another idea is to create your own visual indicator like this:

  .element:focus-visible {
    outline: 2px dashed your color;
    outline-offset: 3px;
  }
  

  • I would make the hero-section-image hidden personally since it is just a vector. Also, again, avoid using words that relates to "graphic" such as "illustration" and others. An img is already an image/graphic so no need to describe it as one.
  • For a form that submits, use method="post" .
  • Also for the input just to post this again:

  if ( input is wrong )
    input.setAttribute("aria-invalid", "true");
    input.setAttribute("aria-describedBy", id of the error-message);
  else 
    input.removeAttribute("aria-invalid");
    input.removeAttribute("aria-describedBy");
  

  The error-message element should have an id attribute which is referenced by the aria-describedBy attribute on the input element. By doing that, your user will know that the input is wrong because of aria-invalid and they will know what kind of error they made because of the aria-describedBy

  • Another idea to implement to further improve accessibility is to have an aria-live element that will announce if the form submission is a success or not. Posting this again for reference for accessible form
  • For the testimonial, I found out yesterday's discussion on the fem slack, you could use this markup:

  <figure>
    <figcaption>
      image in here
      person name in here
      person position in here
    </figcaption>
    <blockqoute>
      quote in here
    </blockqoute>
  </figure>
  

  • Person's image should be visible since it is all about the person so add a alt with their name as the value.
  • Adding cursor: pointer to the button on desktop layout would be great.
  • Also to be honest, I would put aria-label="submit email form" for the button because get started is not really clear right.

  FOOTER

  • Same for the header-logo-website-link use it on the footer as well.
  • When you use aria-hidden="true" on an img, make sure to use alt="" as well since you are hiding it right.
  • Use only 1 ul on those navlinks sine those are related links.
  • Lose the word link on the social-media a tags since a tag is already a link.

  Aside from those, site looks really great again.

  Marked as helpful

  1

  ---

  

  Daniel•250

  @daniel-hennigPosted about 3 years ago

  @pikamart Thank you so much man, you're helping me a lot again! Your suggested function _function is exactly what I was looking for - I've asked for that even in the fem slack, and they gave me good help, but your approach is even better. But I have an issue with that function, though - my terminal says that there is a compilation error (invalid CSS) like this Invalid CSS after "... $value: math": expected expression (e.g. 1px, bold), was ".div($px, 16) + rem". on line 4. Did I do something wrong here? I had to comment it out, so that my SCSS is still running, but as soon as I uncomment it, you'll notice the error.

  Yes, you're right with using the dev tools a lot in order to get the preferred font-sizes - this is what I am already doing, otherwise I wouldn't get all these different values, although it takes a lot of time, but quality pays off :) So I am happy to know, that I am on the right track with that method.

  I've updated the header and footer -logo, I hope this is good? For the footer logo, I've used title id="footer-logosvg" so that I won't have two equal id's, but since it is for the user, it's not quite a good ID, is it (same question, I have for the input1 and input2 - it feels wrong to use these kinds of id's)? Furthermore, I prefer using SVG's as inline-code rather than as the src for the img, because I couldn't find a way to change both the color or rather fill and the size of it with CSS unless it is inline (and I actually don't want to change the SVG itself, because that would mean, that I would have to use multiple logo.svg files then).

  The outlines are a customized now, very good idea.

  The illustrations got rid of the "illustration", as you say, that it truly makes no sense to describe an illustration as illustration or graphic.

  I don't know why I put the get method on the form, but you're right, it is post, since its purpose is to send data📬

  Regarding aria-live: Awesome resource, thank you! This is what I'll definitely consider in my next project.

  Thank you for helping me on the testimonial-card, as well! I've applied the markup on it, but just reversed, because of the order. Does this have a negative effect on anything, or is that order okay, too?

  For the footer, I've merged the two link-groups into one ul and separated each of them with a div, but this leads to a W3C error. Is there a better practice to separate these two groups from each other? I mean, they are related, but they are still two different groups in the design, that's why I've used two separated ul's, so that I can style them with CSS flexbox easily.

  Again, I am truly grateful for your advice!🙏

  PS: Unfortunately, I can't mark your answer as helpful, because there is an error occurring (bug). Well, I see, it's actually working now, but I still got an error message. I'll let Matt know about this 😁

  0

  

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  @daniel-hennig Hey, really great that you find it useful. I rarely get this kind of long reply though hahaha.

  For the scss, that's weird, I don't get that error whenever I use it. Looking at your styles.scss, have you tried using @use functions instead of import. Because what I think is happening, because when you use import on css, it kinds of just include those modules like they are all together now.

  Meaning in my function right, I have rem function, then on your usage of just regular css, you have like 1rem I think it mistook for that. When using @use you will have a namespace, that is why I use @use "functions" as func;

  So that there will be no name collision even if I use 1rem or any value that uses rem unit, because when using the function, you will just use:

  font-size: func.rem(28);
  font-sizes: func.fluid(16, 2vw, 28);
  

  Also since you are using scss it is a good practice to use @use as well.

  For the logo, that's fine to make a separate or you could just reference the first title. Like having that 1 title inside the header-logo, then use that title-id on the footer-logo. I think that will work just fine.

  For the article, yep that is totally fine. Just remember that figcaption should be the first or last item inside a figure. I learn this from Vanza^^

  For the footer-navlinks, you shouldn't have done that markup. Using ul, only li are valid direct child of it and no other elements. Use only 1 ul and have 7 li.

  To place them, you should use grid, then target those selectors using pseudoselectors like:

  li:nth-of-type(5),
  li:nth-of-type(6),
  li:last-of-type {
    grid-column: 2 / 3
  }
  
  li:nth-of-type(5) {
    grid-row: 1 / 2
  }
  ....
  ......
  

  Lastly, for that accessibility issue, you could add aria-label="primary" on the nav inside the header. This way it is unique and labelled properly. So when screen-reader navigate through that element, it will say "primary navigation landmark". You could add as well secondary` on the footer-nav I think.

  Let me know if there are still errors^^

  Marked as helpful

  1

  

  Daniel•250

  @daniel-hennigPosted about 3 years ago

  @pikamart As always, awesome feedback, Raymart. Yeah, you're getting this kind of long replies from me, because I recognize the value of your suggestions, it's just fair IMO :D

  I have changed @import to @use within style.scss and global.scss, then it doesn't show any errors anymore. But as soon as I try to apply font-size: func.rem(24); on line 57 in globals.scss I get the error again. I think I did something wrong with @use, but what is it, exactly? I've seen in your projects, that you use a lot of @use, rather than @import, but if I change all my "imports" to "use", then my whole styles crash. I have to admit, though, that I've never used @use before, therefore I have to study about that, for sure.

  The footer nav ul li is now much more organized. I've worked with grid, instead of with flex, just as you recommended. I used quite a lot of lines in SCSS for this, but it was the fastest/ easiest solution for now. With more time, I would have come across with fewer lines, of course 😎 HOWEVER, there's a big lesson for me again, which I will manifest into the next project! 🥳

  aria-label: primary and ...secondary is another mind-blowing hint. Very useful, again!

  I've learned so much from you in such a short time. Can't believe that skills are on all-time-high now and going to the moon.😊

  0

  

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  @daniel-hennig That's really weird.

  Wait, are you using the live sass extension on the vs-code? I don't think my code works for the since it uses a different version of sass (node-sass). I use gulp-dart-sass for a regular website and when using like React, I don't use a build-setup like gulp.

  But let me make a function that uses live-sass-extension maybe this afternoon if I don't forget about it hahaha. Also if you want to use gulp. You could use my build setup, I make it simpler since I just target scss on my setup.

  By the way, try using the live-sass-compiler of Glenn Marks and not by Ritwick Dey on the vs-code, since the former is not being maintained by now.

  Also, great for those other tips that you newly found useful!

  Be back on this after doing the function if i remember hahaha

  Marked as helpful

  1

  

  Daniel•250

  @daniel-hennigPosted about 3 years ago

  @pikamart yeah that would be outstanding, if you can do the function for me😍

  Yes I'm using the live-sass-compiler with vs-code, but I don't use nodeJS, yet. Using nodeJS and gulp is like my next step of developing my skills, along with themekit and learning to become a Shopify Developer, besides of a PSD to HTML Dev. Therefore this topic here comes just in time hahaha!!🥳

  I don't know of whom my live-sass-compiler is right now, but I'll figure it out and then make sure that it is by Glen Marks. Thank you Raymart😊

  0