Responsive landing page using CSS flex box

Community feedback

• 

  Daniele Manca•160

  @danielemanca1983Posted almost 2 years ago

  Hi Yusuf, Here is some feedback:

  1. Your solution's mark-up code could greatly improve by using semantic mark-up tags, as someone else already mentioned, you could wrap the content in a <main> tag. It is preferable to use semantic html tags instead of aria roles.

  2. I would have enclosed the actual component inside a <article> tag instead, by perhaps assigning a class to it such as card and then all the inner children elements such as the image and text could use BEM methodology for their class names. You can take a look at what BEM is on this link about [BEM] (https://getbem.com/introduction)

  3. The image could be enclosed inside a <picture> tag for semantics.

  4. Typography: the font-family in your solution does not seem to match the ones in the original design.

  5. For the button you have actually used a <p> tag, you must used either an anchor or a <button> for that purpose as the element on the design is a call to action. It also does not have a drop-shadow as per the design.

  6. Cancel the order should also be using an anchor tag instead, as it also is a call to action. In general we would use a <p> tag only for regular text elements.

  7. I would suggest the use of classes instead of IDs, unless we need an ID to bind the html element to a piece of javascript code in order to add some interactive functionality to it.

  It may seem like a lot, but believe me, as I am a front-end developer, I would get this sort of feedback whenever demoing a completed project to the visual designers, if there were mismatching elements.

  Marked as helpful

  0

  ---

  

  Yusuf Babatunde•150

  @owocodedPosted almost 2 years ago

  @danielemanca1983 thank you very much, will surely work on it

  1
• 

  Whale Well Well•50

  @WhaleWellWellPosted almost 2 years ago

  Hi! Overall great project and code, but here is some advice I think you will find helpful:

  • I see that there is a div with a class of main: <div class="main">. To make this site better for screen readers you could add the role of main to your div or wrap your div in a <main> tag.

  <main>
    <div class="main">
      CODE
    </div>
  </main>
  

  or:

  <div role="main" class="main">
    CODE
  </div>
  

  It is suggested you use the <main> tag but either one will work. MDN docs explain it better than me :)

  • I see you have an <h4> in your order component for your smaller heading. I recommend you just use an <h2> instead because the heading tags are mainly for your site's outline and semantic order and not the for the style. If you want your text to look about the size of an <h4> that is fine, you can make it look like that in your CSS. If you want took look farther into this here is a the MDN docs on the heading tags. It has the explanation under "Usage Notes"

  Hope you find my advice useful, WhaleWellWell

  Marked as helpful

  0