@PhoenixDev22
Posted
Hello romila,
Congratulation on completing this challenge. Great work! I have few suggestions regarding your solution, if you don't mind:
HTML
-
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 you can use<a>. For the same reason , you can use<a>to wrapEquilibrium #3429andJules Wyvern. -
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="same-line", you can use an unordered list<ul>, in each<li>, there should be<img>and<p>that way you can align them centerally. -
If you wish to draw a horizontal line, you should do so using appropriate CSS. You may remove the
<div class="border-bottom"></div>, you can use border-top: to the avatar's part. -
To use more semantic tags , you may use
<figure>and<figcaption>for the avatar's part.
CSS:
-
Consider using rem 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.
-
As the image is not
display: block, likely that's why you needed to useheight: 98%to the pseudo element. -
Remember a css reset on every project. That will do things like set the images to display block and make all browsers display elements the same.
Overall , your solution is good. Hopefully this feedback helps.
Marked as helpful
@romila2003
Posted
@PhoenixDev22 Thank you for the feedback, I really appreciate it. I took those suggestions into consideration and changed my code and have updated it so thanks again.
@PhoenixDev22
Posted
@romila2003
Hi Romila again, You're welcome , Glad it was helpful. I have looked into your solution again. By the way , Every developer has his own vision,
-1. I would think that the <h1><a>..</a></h1> approach is more conventional though that way only the text in the title will be clickable.
-2. As you chose a button for the NFT image, so the alt text should say what the button does.
-3. Also , I would not use <section> for only a part of the component as all the content of the card is related .
Hopefully this helps. Happy coding
Marked as helpful
@romila2003
Posted
@PhoenixDev22 Hi thanks again for the suggestion but I have some questions.
- When you mentioned to include the
alttext, is it necessary to include it within thebuttontag? And if so, what text do you think should be inside it because I'm not sure what to write regarding what the button does.
Sorry, if it's a simple question but I never added an alt for a button before so I was just wondering.
Thanks again for the help.
@PhoenixDev22
Posted
@romila2003
Hi romila, Happy to help and Asking questions is crucial in learning.
I meant the alternate text of the image not within the button and it should indicate the purpose of the button what the button does, like: see the full NFT.
Within the button you may use aria-label attribute to make accessible as NFT button contains an image and no (visible) accompanying text. I would suggest to look up on how to make accessible buttons.
Happy coding!
Marked as helpful
