Design comparison
Solution retrospective
I have updated the design as per the latest comments. Please suggest if any suggestions.
Community feedback
- @PhoenixDev22Posted over 2 years ago
Hello Chetana,
Greay work! Congratulation on completing this challenge.
I have some suggestions regarding your solution:
HTML
-
There should be two landmark elements as children of the body element - a
main(which will be the component ) and afooter(which will be the attribution).<Footer>should not be in the<main >. HTML5 landmark elements are used to improve navigation . -
Page should contain a level-one heading. For future use , use the headers in a chronological order. How you order headings dictates how a screen reader will navigate through them. As you go down a level, the number should increase by one, like a numbered list within an outline. In this challenge , as it’s not a whole page, you can have
<h1>visually hidden withsr-onlyclass and use<h2>forEquilibrium #3429. -
There are so many extra <div>’s, think of semantics elements before
div's. -
Since there's a :hover state on the image and means it's interactive, So there should be an interactive element around them. When you create a component that could be interacted with a user , always remember to include interactive elements like(button, textarea,input, ..)
For this imagine what would happen when you click on the image, there are two possible ways:
1: If clicking the image would show a popup where the user can see the full NFT, here you use
<button>.2:If clicking the image would navigate the user to another page to see the NFT, here use
<a>.-
The link wrapping the equilibrium image should either have
Sr-onlytext, anaria-labeloralttext that says where that link takes you. -
For any decorative images, each img tag should have empty
alt=""and addaria-hidden="true"attributes to make all web assistive technologies such as screen reader ignore those images in(icon-view, icon-ethereum, icon-clock). -
For
class="icon"you can use unordered list<ul>. In each<li>should be<img>and<p>( Not headers ) that way you can align them centrally . -
The avatar's alt should not be empty. You can use the creator's name
Jules Wyvern. Read more how to write an alt text -
To use more semantic tags , you may use
<figure>and<figcaption>for the avatar's part.
CSS:
-
There are so many ways to add the hover effect on the image , The one I would use, using pseudo-elements to change the teal bg color to a hsla. Then opacity can be changed from 0 to 1 on the pseudo element on hover. The icon-view doesn't really need to be in the html, you could do it with css.
-
In order to center the card on the middle of the page , you can use the flex/grid properties and
min-height: 100vhto the<body>add a little padding to the body that way it stops the card from hitting the edges of the screen. -
Consider using rem and em units as they are flexible, specially for font size If your web content font sizes are set in absolute units, such as pixels, the user will not be able to re-size the text or control the font size based on their needs. Relative units “stretch” according to the screen size and/or user’s preferred font size, and work on a large range of devices.
Overall , your solution is good. Hopefully this feedback helps.
Marked as helpful1@Chetana-047Posted over 2 years ago@PhoenixDev22 I'm overwhelmed with the response and grateful to you for such a valuable feedback from your end. It was indeed helpful from the standard of the code to logical explanation of the icon scenario which do makes sense to me. Ill revisit and update my code as per given comments. :)
1 -
- @ripalnakiyaPosted over 2 years ago
-
You have so many unnecessary divs used in your HTML file, remove them to make your code file neat
-
The other way you're looking for is... For the eye icon firstly set its position to absolute and then center it. Set its display property to none and when it is hovered , set its display property to inline (which is default for images)
PS: Go through my code and you can reach me if you find any difficulty
Marked as helpful0@Chetana-047Posted over 2 years ago@ripalll Thank you for your valuable feedback and open to help! I looked back at my codes and realized yes indeed there were many divs used ill revisit and update them. :)
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