@pikapikamart
Posted
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 likealt="room"
. Avoid using or adding words that relates to "graphic" such as "icon, image, picture, logo" on thealt
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 1h1
. The carousel title could use anh2
. But you would need anh1
on a page, well, theh1
would be asr-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 thata
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 aaria-label
on thebutton
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 thearia-label="close hamburger dropdown"
and making thearia-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 theimg
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 addingleft: 15px
it pushes it and making it overflow in the layout. You could applywidth: calc(100% - 15px);
on that selector, or maybe addoverflow: hidden
to something to hide it.
Aside from those, really great job on this.
Marked as helpful
@Carlos-A-P
Posted
@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!