@pikapikamart
Posted
Hey, great work on this one. Desktop layout looks really great, the site is responsive as well, though I early suggest to make the mobile breakpoint a bit sooner because at like point 770px, the image is too thin, so making that transition would prevent that or maybe letting the left-side resize as well. Mobile view looks great also.
Some suggestions would be:
- If you somehow inspect layout in dev tools at the bottom, the image is not occupying the full height of the screen thus its upper part is being hidden. So a quick fix, on your
img
tag, instead of usingheight: 100vh
which is a bad styling, usemin-height: 100vh
so that it will scale properly. - If you use a primary
header
make sure that it is outside themain
tag so that it will be treated as one of you primary landmark. For this one, it would be better to use :
<header />
<main />
With the header
being absolute on the top so that it won't affect the layout of the main
.
- Use only the website name as the website-logo
alt
value. - When using
img
tag, you don't need to add words that relates to "graphic" such as "logo" and others, sinceimg
is already an image so no need to describe it as one. - I would use a descriptive
alt
on the lady's image since if you look at it, the lady is somehow using clothing on the base-apparel, maybe she is showcasing the clothing, those ornaments. - On the
h1
, you don't have to usebr
to make each text wraps on another row. You can usemax-width
on theh1
itself and adjust it until each text are wrapped on their own row. - The
button
inside theform
should be usingtype="submit"
. Remember that when abutton
is placed inside aform
element, it defaults totype="submit"
. So imagine if you have a close-button inside theform
without specifyingtype="button"
click the close-button will submit theform
. Be aware of this kind of scenarios. - The error-icon is only a decorative image. Decorative images should be hidden for screen-reader at all times by using
alt=""
andaria-hidden="true"
to theimg
tag or onlyaria-hidden="true"
if the image is usingsvg
. - Right now, the error is only seen visually but not really linked with the
input
itself. For a proper error, message, it would look like this pseudocode:
if ( input is wrong )
input.setAttribute("aria-invalid", "true");
input.setAttribute("aria-describedBy", id of the error-message);
else
input.removeAttribute("aria-invalid");
input.removeAttribute("aria-describedBy");
The error-message element should have an id
attribute which is referenced by the aria-describedBy
attribute on the input
element. By doing that, your user will know that the input is wrong because of aria-invalid
and they will know what kind of error they made because of the aria-describedBy
.
Have a look at this accessible form snippet that I have On this one, you can see aria-live
is used as well to inform the user right away on what is the status of the form
submission.
Aside from those, great job again on this one.
Marked as helpful
@kenreibman
Posted
@pikamart Absolutely love the feedback! Thank you for taking the time to write this, it helps people implement better practice.
I went ahead and changed some of that styling advice that you gave me, and I will be implementing the aria attributes to my JS later today.