Design comparison
Solution retrospective
Had trouble doing the overlay image while hovering the main image. luckily I found a video regarding the problem. (linked it on the github solution README)
- How is my solution to the hover on main image? Is there a better way to do it?
Thanks for the help! 👍
Community feedback
- @PhoenixDev22Posted over 2 years ago
Hello KurtJF,
Congratulation on completing this challenge. Excellent work! I have few suggestions regarding your solution, if you don't mind:
HTML
- The most important part in this challenge is the interactive element. Since there's a :hover state on the image and means it's interactive, So there should be an interactive element around it. 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>
.- You should have used
<a>
to wrapEquilibrium #3429 and Jules Wyvern
too.
- The link wrapping the equilibrium image (
image-equilibrium
) should either haveSr-only
text, anaria-label
that indicates where the link navigate the user(not describes the image).
- 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
).
- You can use unordered list
<ul>
to wrapclass="price"
. In each<li>
should be<img>
and<p>
, then you may use flex properties to align them centrally.
- If you wish to draw a horizontal line, you should do so using appropriate CSS. You may remove the
<hr>
, you can useborder-top:
to the avatar's part.
- The alternate of the avatar image should not be "picture of nft maker ", it’s meaningless. You can use the creator's name
Jules Wyvern
. Read more how to write an alt text .
- 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. There is no need for a extra clutter in the HTML.
- Adding
rel="noopener"
orrel="noreferrer"
totarget="_blank"
links. When you link to a page on another site usingtarget=”_blank” attribute
, you can expose your site to performance and security issues.
You can have a look at my solution here, it might help. if you have any suggestions , please do leave a feedback.
Hopefully this feedback helps.
Marked as helpful0@KurtJFPosted over 2 years ago@PhoenixDev22 Thank you for the detailed response! I didn't consider making the image pop up on click, I'll include it when i update my solution. I have a few questions if you don't mind.
-
Is it better to wrap an
<h1>
as<a href="#"><h1>Text</h1></a>
or<h1><a href="#" />Text</h1>
? -
Regarding
aria-label
andpseudo-elements ::before and ::after
I just recently learned about them and still haven't fully grasped when or where to use it properly. At least now I know those elements can be used in this situation, Thanks!
I'll include your suggestions when I resubmit my solution, Thanks again! 👍
0@PhoenixDev22Posted over 2 years ago@KurtJF Glad to help.
For your question , it depends on what you are trying to do:
- When you wrap the anchor around the header
<a><h1>...</h1></a>
, the whole line get's clickable. h1 id block element and <a> is inline element. Personally, I wouldn't do it.
- Having the <h1> tag on the outside would make it easier to scan for in source code and only the text will be clickable.
Hopefully this makes it clear. Happy coding!
0
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