@pikapikamart
Posted
Hey, great work on this one. Layout is really good in desktop, the mobile as well is good. The desktop layout could have taken a bit more from the media
because right now, it goes to tablet layout at point 1230+ px, which I think desktop could be present at that. For the mobile layout, it would be great for the 3 tabs don't take full width. Since right now, it is being justify-content: space-between
the 3 tabs are so far away from each other. Would be great to have a max-width
on the flex parent.
I won't look in the javascript since like what you said, you are going to refactor it.
Still, some suggestions:
- The fonts are not applying for the planet links, you might want to check that one out.
- It would be great to have a
header
element that will wrap the upper section, instead of just usingnav
. - The website logo link should not have been inside the
nav
since it is not really a primary navigation link. The structure would be great if:
<header>
<div>
<a href="/" aria-label="homepage earth"> The Planets </a>
</div>
<nav aria-label="primary">
different planet links
</nav>
</header>
The planet links are the ones that is inside the nav
because those are the navigational links. I added as well a aria-label="homepage earth"
on the website logo link because it directs the user in the earth page, better inform user where it would take them.
- There are multiple
div
after theh1
tag. Check it out. - Do not use
tabindex='-1
to an interactive element, this renders it out and won't be accessible at all. Right now, you can't tab on those selections about the current planet. - It would be great as well for the selections of information about the planet, is implemented to be a
tabbed interface
. You might want to search that one out, it might be confusing to implement, but that will be a great accessibility feature on this challenge. - If you click on the
surface
then inspect the image being rendered, you will notice that everyone'salt
is being used asalt="earth"
which should not be, it should be the current planet name as the value being used. You might want to check that one as well. - The bottom part, since those are list of information, it would be great to have them inside a
ul
element.
MOBILE
- Hamburger menu should have been using
button
and not justdiv
withimg
inside it. Usingbutton
will make it more accessible, right now only by click events are triggering it, you can't use keyboards or other assistive tech.
Assuming that button
is used on the hamburger
- The
button
should havearia-label="menu dropdown toggler"
so that users will know what thisbutton
does, it should also have aaria-expanded
being set to it, triggering via javascript, helps user and informing them if it has expanded something. - The
svg
on thebutton
should havearia-hidden="true"
so that it won't be pick up by assistive tech. - It would be great to have a focus trap on the dropdown.
- Also, on mobile, the fonts are not being rendered. Check that one.
Aside from those, really great job.
Marked as helpful