Responsive NFT card

Community feedback

• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted over 2 years ago

  Hi Pradeep Saini,

  Excellent work! I have few suggestions regarding your solution if you don't mind:

  HTML

  • Page should contain a level-one heading. In this challenge , as it’s not a whole page, you can have<h1>visually hidden with sr-only class and use <h2> for Equilibrium #3429.

  • 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>.

  • Also use <a> to wrap Jules Wyvern and Equilibrium #3429

  • The link wrapping the equilibrium image(image-equilibrium ) should either have Sr-only text, an aria-label or alt text that says where that link takes you.

  • For any decorative images, each img tag should have empty alt=""` and add aria-hidden="true"`` attributes to make all web assistive technologies such as screen reader ignore those images in( icon-view, icon-ethereum, icon-clock ).

  • If you wish to draw a horizontal line, you should do so using appropriate CSS. Remove the <hr>, you can use border-top: to the avatar's part

  • The avatar's alt should not be photo of the creator of the nft. You can use the creator's name Jules Wyvern. Read more how to write an alt text

  • You can use unordered list<ul> to wrapclass="price flex"In each <li> should be <img> and <p>, then you may use flex properties to align them centrally.

  • To use more semantic tags , you may use <figure> and <figcaption> for the avatar's part.

  • 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. And the icon view doesn’t really need to be in HTML. You can use CSS, as there’s no need for another clutter in the HTML.

  CSS:

  • To center the card on the middle of the page , you may use flex/grid properties and min-height: 100vh to the body.

  Best practices:

  • Nesting selectors can lead to specificity problems if you nest too deeply. One problem with overly specific selectors is you have to write even more specific selectors to override them

  • Really important to keep css specificity as low/flat as possible. The best way to do that is single class selectors. Classes can be reused

  • 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.

  • line-height: 2.5rem;Use a unitless line-height value to Avoid unexpected results due to inheritance behavior. You can read more in mdn

  • There are so many arguments against setting the root font-size: 62%it state that you should never change the root font size because it harms accessibility.

  Aside these, Your solution is good. Hopefully this feedback helps.

  Marked as helpful

  2

  ---

  

  Pradeep Saini•990

  @pradeeps4iniPosted over 2 years ago

  @PhoenixDev22 Hello. How are you?

  Thanks a lot for the detailed feedback and suggestions to improve the code. I am going to work through each of them.

  Where can i learn about practices one should follow to write good CSS?

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @pradeeps4ini CSS Guidelines by Harry Roberts is a good guideline to follow. But, there are a lot of rules so I recommend implementing them slowly.

  In general,

  • I highly recommend writing the styling using the mobile-first approach. It often leads to shorter and better performance code. Also, mobile users won't be required to process all of the desktop styles.
  • Use single class selectors for styling whenever possible instead of id. id has high specificity which can lead to a lot of issues on the larger project. It's best to keep the CSS specificity as low and flat as possible. 😉

  0

  

  Pradeep Saini•990

  @pradeeps4iniPosted over 2 years ago

  @vanzasetia Thanks for the link and suggestions.:)

  2

  

  PhoenixDev22•16,950

  @PhoenixDev22Posted over 2 years ago

  @pradeeps4ini

  You're welcome. Glad it was helpful.

  For a starter , you may start with this article And @vanzasetia have already provided an amazing source CSS Guidelines by Harry Roberts. Happy Coding!

  1
• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  Hi, Pradeep Saini! 👋

  Good effort on this challenge! 👍

  You already get a lot of feedback from @PhoenixDev22 (which I recommend following). I am just going to add two more suggestions.

  • Firstly, I suggest making the img as block element and set max-width: 100% to remove the tiny whitespace below the Equilibrium image. Also, in general styling the img element this way will make it easier to work with img element.
  • Secondly, I suggest adding rel="noopener" to any anchor tags that have target="_blank". This is a security essential for external links. I suggest reading the web.dev article to learn more about this.

  That's it! Hope this helps. Happy coding! 😄

  Marked as helpful

  1

  ---

  

  Pradeep Saini•990

  @pradeeps4iniPosted over 2 years ago

  @vanzasetia hello.

  I am going to work on the suggestions you provided. Thanks a lot for them.

  Both, you and @PhoenixDev22 are amazing.

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @pradeeps4ini Thanks, Pradeep Saini! I appreciate your kind words! 😄

  0