Design comparison
Community feedback
- @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 - have at least one
<h1>in your code
Good job! :)
Marked as helpful1@Hamza-NoahPosted almost 3 years ago@anoshaahmed Ok I wrapped everything with a <main> but the h1 should be the title of the page there is no place to put it either the card title so should I replace h2 with h1?
1@anoshaahmedPosted almost 3 years ago@Hamza-Noah Yuppp, make ur h2 the h1. And don't forget to go to your Report and generate a new one!
Also while you're doing this, might as well change the color of "Equilibrium #3429" and "Creation of Jules Wyvern" to white.
Marked as helpful1@Hamza-NoahPosted almost 3 years ago@anoshaahmed ok this means I have to remove the hover state from the Equilibrium #3429" and "Creation of Jules Wyvern" to white. right?
1@anoshaahmedPosted almost 3 years ago@Hamza-Noah
No, you don't need to remove the hover state.
They're supposed to turn blue when they're hovered. And you achieved that result, so good job on that.
You simply just need to add
color: white;toh2 {}and to.account {}1@Hamza-NoahPosted almost 3 years ago@anoshaahmed Thank you for your help and feedback I will take into my consideration those mistakes to avoid them in the future thanks again
1 - wrap everything in your body in
- @grace-snowPosted almost 3 years ago
Damn I just wrote loads of feedback on css too then lost it 😭
Oh well I'll try to summarise but it will be briefer...
- use min-height 100vh not height
- use max-width on the card not width
- theres no need for the eye image to be in css when you are already using a pseudo element for the image hover effect. You can place that image in the pseudo as a background-image and get rid of the transform-translate stuff to center it as background images are positioned centrally by default anyway
- stop nesting css selectors. I can't stress how important this is. Ykh will have nightmares on larger projects writing css like this. Use single class selectors as much as possible, keep your css specificity low
- add padding to body or margin to card to keep the component from hitting screen edges on mobile
Sorry for the rush - I hope that helps anyway. Good luck
Marked as helpful1 - @grace-snowPosted almost 3 years ago
This doesn't look finished yet I'm afraid. The solution doesn't match the design with content hitting (and going off) screen edges on mobile and some text a dark color on dark background, making it unreadable.
Also, I can't see the hover states because you haven't used interactive elements.
First thing you need to do is fix the html
- use interactive elements (in this case anchor tags) for anything that should have interactive behaviour (hover styles indicate interactivity)
- make sure those links are labelled once added - that means the image one will need alt text or its linn will need aria-label / aria-labelledby attributes or Sr-only text inside
- never have text in divs or spans alone, always use meaningful elements
- really, you need to use landmark elements on every webpage you do. That's what the warnings are about in your Report. This isn't a full webpage so it may be ok to skip, but it does no harm adding in a main element to wrap your component.
Marked as helpful1@Hamza-NoahPosted almost 3 years ago@grace-snow what is the interactive elements I have to use to make the hover state ?
0@Hamza-NoahPosted almost 3 years ago@grace-snow I can not thank you enough without your notes I could never know my mistakes and will repeat them again unknowingly your feedback will help me avoid those mistakes in the next challenge special thanks for your help
0
Please log in to post a comment
Log in with GitHub
Join 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