Design comparison
Solution retrospective
Thank you. Feedbacks are welcome.
Community feedback
- @grace-snowPosted almost 3 years ago
Hello
Don't use role on divs! Use the correct element. There's no reason why you can't use main and footer (for attribution) on this.
Similarly, don't use the old deprecated input type submit when you can use a button element.
I don't think the button is the right choice for the purple box. What is it you expect that to do? That is just intro text for the form, a paragraph
For this form there is a few essential changes needed, as with every form you do in future
- inputs always need labels. Sr only text with aria labelledby would be a good choice, or you could use aria-label but be aware that won't always translate for users speaking other languages
- optional but recommended - make the error icons into background images on the input. General rule is don't clutter the html with elements that don't need to be there
- every error message element needs an aria live attribute
- every error message needs a unique id
- every input with validation needs a required attribute
- every input with validation needs an aria describedby attribute pointing to the ID of its error message
- the form needs a novalidate attribute as you are doing custom validation here
Marked as helpful1@JesseOlisaPosted almost 3 years agoHello, Grace
Thank you so much for the feedback. I have learnt a lot from this.
I really appreciate.
1 - @anoshaahmedPosted almost 3 years ago
To get rid of the accessibility/HTML issues shown in your Report:
- wrap everything in your body in
<main>
OR use semantic tags OR giverole=""
to the direct children of your<body>
... Click here to read more - add
alt=""
to your<img>
tags - use
<a>
only, don't use<button>
.
Good job! :)
Marked as helpful1 - wrap everything in your body in
- @astrageniusPosted almost 3 years ago
Hello Jesse,
First
Change the min-height property of the body to 100vh so the background color can completely expand.
body { min-height: 100%; min-width: 100%; position: relative to min-height: 100vh; background-color: hsl(0, 100%, 74%); }
Because with min-height or min-width to 100% the container will only take the space he really need.
min-height: 100%; min-width: 100%;
You can also delete
.pink-overlay { content: ""; top: 0; left: 0; position: absolute; min-width: 100%; min-height: 100%; z-index: -1;
You dont need it anymore.
Change the background-color in the media-queries also.
Second
center the Sign up form.
Add display grid to the body
body { display: grid; justify-content: center; align-items: end; }
Your Sign up Form should now in the center of the screen.
That's it :) Hope i could help you. Have a nice day and a Happy new Year :)
Marked as helpful1@JesseOlisaPosted almost 3 years agoHello@astragenius,
Thank you so much for the feedback. I will make those corrections now. I really appreciate.
And Happy New Year in advance. :)
0 - @grace-snowPosted almost 3 years ago
You're making the js more complex than it needs to be here
- you don't need to change placeholder text in js. It will always show up if input is empty
- you don't need and shouldn't change any inline styles with js
- you don't need to get multiple elements by classes beyond the form inputs
- you should be using const not let
- instead of toggling so much in js, all you need to do is toggle one class on an element that wraps the input, img and error. Everything else you're doing can be changed in css based off the presence of that class. Eg
.has-error input { // background error image, red border etc } .has-error error-message { // display block etc }
It's one of the few times where you want to use nested css selectors
0@JesseOlisaPosted almost 3 years ago@grace-snow
• I changed the placeholder text because that was what I saw in the design given. So, I decided to try it.
Thank you so much Grace, I will take all these corrections on the JS part.
Please can you elaborate more on the last point. Thank you.
0@grace-snowPosted almost 3 years ago@JesseOlisa what is it that isn't clear?
Change html so there is a class to hook into on each input wrapper div
<div class="js-input-wrapper"> <input type="text"....... > <img src="........> <div class="error.........></div> </div>
For each input wrapper add an has-error class if there is an error, remove the class otherwise.
In css use the presence of that class on the wrapper to style what you need for the error
Marked as helpful0@JesseOlisaPosted almost 3 years ago@grace-snow It's very clear now. Thanks a lot!! I appreciate.
1
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord