Responsive, Mobile First Approach Using Flex, Grid, CSS Variables

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  Hello again, Arthur! 👋

  It's already two weeks since my last feedback to you. It's great to see you finishing another challenge! 😄

  Great work on this challenge! Your solution looks pretty good! 😀

  However, I thought that there are things that can be done better.

  • The header should only contain the logo image. For the image-hero, you can make it as a background-image instead. The same thing for the image-hero for desktop layout.
  • The most important content on the page is the logo and surely not one with blank alt="" text. Without that information, a screen reader user or a search engine wouldn't know what the site/project is called.
  • For the download buttons, each of them should be an anchor tag with download attribute instead of a button element. Again, like I said in the previous feedback if you ever going to use button element always specify the type of the button. In this case, set the type of them as type="button". It's going to prevent the button from behaving unexpectedly.
  • For the section-number, you can create it using pseudo-elements. I had created using pseudo-elements and it's possible.
  • All woman and man images are decorative so leave the alt empty.
  • The overline should be a paragraph element not h3 and also, remember that headings must be used in logical order.
  • I recommend making the footer fill the entire page to make it looks more similar to the design.

  Overall great work. I suggest to keep writing the styling with the mobile-first approach and in addition to that, I recommend using em unit for the breakpoint unit. Here's an article that will explain the reasons.

  I hope this helps! Keep up the good work! 👍

  Marked as helpful

  1

  ---

  

  P

  Arthur•290

  @ArthurPogPosted over 2 years ago

  @vanzasetia As always, great advice, Vanza! I'll get to fixing it first thing in the evening.

  To be honest, everything was going fine then I got completely stuck at the backgrounds. That little overflow on the sides just destroyed me. The project took more than 9 hours to complete because of it and by the end I was moving the HTML code left and right, desperate to get it working and go to sleep. It was 5 a.m. xD

  When I get back to it this evening I will definitely clean it up per your advice.

  So, thank you once again, for your input. It makes me code better, cleaner and understand what I am doing more.

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @ArthurPog You're welcome, Arthur! 👍

  Background images were also the most difficult problem for me when I first started doing Frontend Mentor challenges. 😅

  Glad to know that my feedback was helpful to you. 😄

  1

  

  P

  Arthur•290

  @ArthurPogPosted over 2 years ago

  @vanzasetia Well. Hopefully it's fixed now. I simply could not make the div of the header image to resize based on the height of the background image. I used a nifty trick to accomplish that but did not bother with the hassle for the desktop version and just hid it from ARIA xD If only they brought in a background-overflow and container width and height value that listens to the background-image height and/or width property! That would make this partially hidden backgrounds so much easier to make.

  The alt's were fixed and so were the buttons, footer and h3 elements! Thank you for the distinction between buttons and anchors. From now on I will be using buttons for functions within the webpage and anchors for download links and links leading out of the webpage.

  I also tried commenting my code for the first time. Maybe I went overboard with how verbose I was.

  My man, and the ::after and ::before pseudo-elements?! You blew my mind again. Had no idea that was a thing and now I can't imagine being without it! Will definitely use it in my next project.

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @ArthurPog Great job with the update! 👍

  Regarding the alternative text of the logo image, I suggest removing the word "logo". It's already an img element so there's no need to specify it as an image within the alternative text. Also, just like normal users that can know that it is a logo (by just seeing it) the same thing for screen reader users.

  For the comment, I suggest only adding comments for things that "unclear". For me, when I did something "hacky" in my CSS, like center the icon vertically, I had to add top: -0.2rem then I would add a comment to it.

  /*
   * To center the icon vertically because
   * unfortunately the icon is not perfectly
   * vertically center with just flexbox
   */
  .button__icon--ios {
    position: absolute;
    top: -0.2rem;
  }
  

  So, if in the future I come back to this project and I am asking, "Why does the icon need to be absolute positioning?" Then I can simply see the comment that I wrote.

  I think you've done a great job with the hero image by telling what is the state of your mind at that time so, that when your future self or other developers want to work with your code, they know why it needs to be like that.

  Marked as helpful

  1

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @ArthurPog Also, don't forget fo fix all the issues that has been reported.

  0

  

  P

  Arthur•290

  @ArthurPogPosted over 2 years ago

  @vanzasetia Just checked the list. Did I miss anything?

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @ArthurPog Have you generated a new report? There are still 1 HTML issue and 2 accessibility issues.

  Marked as helpful

  0

  

  P

  Arthur•290

  @ArthurPogPosted over 2 years ago

  @vanzasetia Done. Completely forgot about that :)

  0