@pikapikamart
Posted
Hey, awesome for another challenge. The desktop layout looks fine, though the hero-image is quite big/long and making the site a bit longer because of some white-spaces as well. The site is responsive but if you go at 340px above, you will notice that the site is hiding the content of the page and creates horizontal scrollbar.
MordenWebDev already gave a feedback on this one, just going to add some suggestions as well:
- Avoid using
height: 100vh
on a large container like thebody
as this makes the element's height capped based on the viewport/screen's height. Instead usemin-height: 100vh
so that the element will expand if it needs to. - Again, a page needs to have a
main
tag to wrap the main-content so that user will be able to quickly navigate the content using landmark elements. Usemain
on the.container
. - Remember that a website-logo is one of the meaningful images on a site so use proper
alt
for it. Use the website's name as the value likealt="ping"
. - Your
input
right now currently lacks associatedlabel
to it or anaria-label
to which will define the purpose of theinput
element. Always include it so that user will know what they need to give on eachinput
. Make sure thatlabel
is pointing to theid
of theinput
as well. - The notify-button should be using a
type="submit"
attribute. 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"
clicking the close-button will submit theform
. Be aware of this kind of scenarios. - Right now, the error message or the error in general is only seen visually but not really linked to the
input
properly. A proper way of adding errors would look something 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
. If you like, have a look at this simple accessible form snippet that I have. Let me know if you have any queries about this one.
- For the hero-image, you could use a more descriptive
alt
, it could be something like `ping's application user dashboard" or if you could come up with a more descriptive. - Those social-media links could be inside a
ul
element since those are "list" of links. - Each
a
tag that wraps the social-media icon should have eitheraria-label
attribute orsr-only
text inside it, defining where the link would take them. For example, you should usefacebook
as the value if the link would take the user to facebook. - Also, I would put the social-medias inside the
footer
since those aren't really part of the main-content of the page. - Lastly, just reducing some sizes perhaps of the image and some white-spaces:>
Aside from those, great job again on this one.
@mahnoork18
Posted
@pikapikamart thankyou for making me realise my small mistakes while coding. just wanted to know the code snippet you shared doesn't have emailValidation, I think my js code is working fine but I will definitely look into it. thanks
@pikapikamart
Posted
@mahnoork18 Hey, glad that you found my review useful^^
For the snippet, there is a email validation as well on that one, just using a simple regex on the testEmail
function.
Though that whole code snippet is more likely to be seen as adding necessary error attributes:>