HTML, CSS, JavaScript

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. The layout in desktop is really good, the mobile is good but like others said, the resizing state, layout is not really holding the whole content like it should do, leaving empty spaces on the left and right side.

  Additions only to what others have said, suggestions:

  • On this one structure, I would have a header that will take the navbar. It would be like:

  <header />
  <main />
  

  But you might think, " how would you use header since the navbar links holder is like binded to the carousel image container" position: absolute :>. Making the header absolute to position up top, that way, the image behind it won't be able to affect the header even if the image changes since it is a carousel.

  • The website logo should use its name as the alt value like alt="room". Avoid using or adding words that relates to "graphic" such as "icon, image, picture, logo" on the alt attribute. Assistive tech will handle those for you.
  • Also the flashing of images, not really a fan :> (can cause some effects on the eye).
  • Avoid using multiple h1 on a webpage. Use only at least 1 h1. The carousel title could use an h2. But you would need an h1 on a page, well, the h1 would be a sr-only element, it would be like:

   <h1 class="sr-only">frontendmentor furniture collections</h1>
  

  We will just make the h1 only visible for screen reader users.

  • Adding a visual indicator on the carousel button would be really good. Right now, if you tab on them, you don't have any indicator where you are at, adding those will be really good.
  • The shop now -> you don't have to separate those two, the arrow-icon could be a :after selector of that a tag. That way, there will be only 1 link like it should be.
  • Also, making an accessible carousel is hard but it can be done, I don't have any resource by now, but just letting me remind you about carousel accessibility, in case you are going to develop more in the future.

  MOBILE

  • The hamburger menu toggler needs to have a visual indicator as well. Adding some on the :focus-visible will be really great.
  • The hamburger menu as well should have a aria-expanded attribute on it. This will allow a user to be informed that it has expanded something, which is the navbar dropdown. Add as well a aria-label on the button like:

  <button aria-expanded="false" aria-label="hamburger dropdown toggle" /> # expanded false at default
  

  • Closing button on the dropdown as well should have visual indicator along with the the aria-label="close hamburger dropdown" and making the aria-expanded to false. You might be confused on this one, that is why you should add 2 aria-expanded to both button, but same value for each.
  • You should also used alt="" on the img for the hamburger menu and closing dropdown since you don't want the user to navigate directly with that image.
  • There is a horizontal scrollbar on the mobile state, what causes this is the .hamburger-menu selector, since you are adding left: 15px it pushes it and making it overflow in the layout. You could apply width: calc(100% - 15px); on that selector, or maybe add overflow: hidden to something to hide it.

  Aside from those, really great job on this.

  Marked as helpful

  1

  ---

  

  Carlos•1,110

  @Carlos-A-PPosted about 3 years ago

  @pikamart I really appreciate your feedback and suggestions, you actually helped resolve some issues I was having and wasn't sure how to fix them. Thanks again!

  0
• Account deleted

  Hi,

  At first glance I think it looks ok, and does what it's supposed to be doing.

  • This isn't an easy project to make responsive, which is why I think you resorted to an early switch to mobile view at 1260px.(which I think is a bad idea)

  • Which leaves a huge amount of unused space on the sides, and I don't think that is a good thing.

  Keep Coding👍

  Marked as helpful

  1

  ---

  

  Carlos•1,110

  @Carlos-A-PPosted about 3 years ago

  @thulanigamtee, thanks for the feedback. .. That was my next task, as I was mostly glad that I was just able to make it work, aha.

  0
• 

  ZaraAg•145

  @zaraag92Posted about 3 years ago

  Congrats on finishing this project.

  It looks nice, but as for me you can add fade animation to only images, so the menu will be fixed and animation will affect only images.

  And I'll do more responsive. By the way you can check mine too. I just finished it.

  2