Responsive landing page using html, css flex&grid and js

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. Desktop layout looks fine, just needed a bit of padding here and there and the 4 items on the bottom part are not using the whole width and I am seeing an inconsistent units right now inspecting on dev tools. The responsive state could be better since the 4 items again are not scaling well, the mobile layout is fine but some contents are not being centered properly.

  Fidel Lim already gave great feedback on this, just going to add some suggestions on this one:

  • Avoid using vh unit in height property in general as this is not consistent. Instead use rem value on this if you want. I seen it on the .hero and footer section and haven't check on other elements as well, but avoid this one.
  • A typical structure of a site would look like:

  <header />
  <main />
  <footer />
  

  This way, all element that has content would be inside a landmark element.

  • The nav should be inside the header if you are going to refactor this one.
  • Website-logo-img should be using its name as the alt value like alt="sunnyside" remember that a website-logo is one of the meaningful element of a site so always use proper alt on it.
  • The arrow-icon should be hidden since it is decorative image only so use alt="' and add aria-hidden="true" attribute on it.
  • Avoid using multiple h1 on a page, use only at least 1 per page. Change those into other heading tags.
  • font-family is not being applied correctly, you might want to check that one out.
  • Other decorative images could have use extra aria-hidden="true" on it.

  ( As inspecting it and scrolling while dev tools at the bottom, large chunks are being hidden, overlapping one another because of vh unit usage)

  • 4 images at the bottom should be hidden since those are only decorative images.

  FOOTER ( Same, some chunks are being hidden, cant see the social media if dev tools opened up )

  • 3 links should be using a tag and you can nest those inside a ul tag which is inside a nav since those are still your website's navigational links.
  • Those social media should be using a tag on each since they are "links". Nesting them inside ul would be great as well.
  • Each a tag that would wrap the social media should be using either aria-label attribute or a screen-reader element inside it. The value for whatever method you will use should be the name of the social media like aria-label="facebook" on the facebook link. This way, user will know where this link would take them.
  • Each social media img should be hidden as well since they are only decorative. Use the method I mentioned above on hiding elements.

  MOBILE

  • Hamburger should be using a button element and not only div. Remember that interactive component should be using interactive elements.

  SUPPOSING BUTTON IS USED

  • The img inside it should be hidden.
  • The button should have the same method with aria-label or screen-reader element. The value will describe on what the button does, you could make use of like aria-label="navigational dropdown menu" on the button tag.
  • As default, the button should have aria-expanded="false" attribute on it, it will be set to true if the user toggles the button, use javascript on this one. You will be using .setAttribute method and vice versa to make it false.

  Aside from those ,still great work on this one. If you have queries just drop it here okay.

  Marked as helpful

  2

  ---

  

  Kurt Gonzales•300

  @KurtGonzalesPosted about 3 years ago

  @pikamart Thank you so much for your effort on reviewing my code! it helps a lot, sir!

  0
• 

  Fidel Lim•2,775

  @fidellimPosted about 3 years ago

  Hi Kurt,

  You could also try to look at the accessibility issues in your project report. So that you can have an idea of how you could fix the issue.

  Marked as helpful

  0
• 

  Fidel Lim•2,775

  @fidellimPosted about 3 years ago

  Hi Kurt,

  Great job on finishing your first responsive navigation project! Just some suggestions I would like to share with you are the following:

  • your div class="gallery', it is better to set the width using "1 fr" so that they are of the same width whatever the size of the screen is.
  • for client 1,2, and 3 classes, it is better to add display flex. So that you can center the content of them. For instance, the image is not centered well. This will fix the issue. If you would like to learn .about flexbox, have a look at this link.

  I hope it helps! Good luck!

  Marked as helpful

  0

  ---

  

  Kurt Gonzales•300

  @KurtGonzalesPosted about 3 years ago

  @fidellim thank you for the feedback, sir!

  1