Hey, awesome work on this one. Desktop layout looks really great, it is responsive and the mobile state looks great as well.
Some suggestions would be:
- First, you should have nest the
sectionon the markup inside the
mainsince it is one of the main content of the page and because it should be inside a landmark element.
- Avoid using
idattribute as a selector in css because it is a bad practice due to css specificity. Use
classto target elements.
- Website-logo should not be inside the
navsince it is not being treated as a link, use
atag on it with an appropriate
aria-labeland use it on the
imgshould be using the website's name as the
alt="loopstudios". Remember that a website's logo is meaningful so always make sure it uses the proper
- Also when using
altattribute, avoid using words that relates to "graphic" such as "logo" and others. An
imgis already an image/graphic so no need to describe it as one.
- Do not directly type the wordings as uppercase on the markup, if you do this, screen-reader will read the text letter-by-letter and not by the wordings. Use only the lowercase version to write in the markup and instead use
text-transform: uppercaseon it.
- Other images that are used could have use a better
altbecause what does
- When wrapping a text-content do not just use
spanto wrap it, use meaningful element like a
ptag if it just a regular text or heading tag if it is an heading.
see allshould be using
atag since it will be more like a link on a real site, a page-link where users can "see all" creations.
- You could use
ulon the creations since those are "list" of creations.
- Also, you could have use
atag inside the creation's title since again, on a real site, it would be a link to view a single creation.
- Same for the website-logo it should use the website name as the
- Links below the logo could have use
navsince those are still your website's navigational links.
atag that wraps social media, it should have either
aria-labelattribute or 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
atag. This way, users will know where this link would take them.
imgfor the social-medias should be hidden since those are only decorative images, so use
- Remove the
width: 100vwon the
#banneras this causes horizontal scroll.
- Hamburger menu should be using a
buttonelement since it is a control. Again, interactive components uses interactive elements. By using
divyou are making it not-accessible.
SUPPOSING BUTTON IS USED
buttonwill be using the method I mentioned using
aria-labelattribute or screen-reader element inside. The value will describe what does the
buttondo. The value could be
aria-label="navigational dropdown menu".
buttonshould be hidden, use the method I mentioned above.
buttonshould have a default
aria-expanded="false"attribute on it. It will be set to
trueif the user toggles the
Aside from those, great work again on this one.
Marked as helpful
@pikamart Thank you for the detailed review! I noticed some of them after submitting. Though new things learnt regarding accessbility by your comment.
For hamburger menu, in this project I wanted go with full CSS, hence I used checkbox instead button. But I guess it would be better to use aria-label and tabindex on the label for accessbility. In the long run I'll be using button with JS for toggle menu. This was more of a challenge when I saw in the brief it could be done without JS.
Anyways thanks for your time this was really helpful :)