NTF Card Component with Grid and Flexbox

Community feedback

• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted over 2 years ago

  Hello Freddy Santana,

  Excellent work! Congratulation on completing this challenge. I have some 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 use <a>.

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

  • The link wrapping the equilibrium image (image-equilibrium ) should either have Sr-only text, an aria-labelor alt text of the image that indicates where the link navigate the user(not describes the image).

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

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

  *The avatar's alt should not be creator, it’s meaningless. You can use the creator's name Jules Wyvern. Read more how to write an alt text . For future use, the alternate text should not be hyphenated, it should be human readable.

  • 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.The icon view doesn’t really need to be in HTML as there is no need for a extra clutter in the HTML. You can have a look at my solution here, it might help.

  CSS:

  • Add min-height: 100vh instead of height: 100vhto the body that let the body grows taller if the content outgrows the visible page instead.

  • height: 600px; It's not recommended to set height to component, let the content of the component define the height.

  • width: 350px;an explicit width is not a good way. consider using max-width to the card inrem instead.

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

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

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

  Marked as helpful

  1

  ---

  

  P

  Freddy Santana•190

  @CreixzPosted over 2 years ago

  @PhoenixDev22 Thanks my friend, I saw your code and I think it's very well done, It's really helped me to realize some things especially change the <div> by the <ul> or even use <figure>also, if I had to give you something, maybe you could add BEM, or maybe you didn't implement it for some reason. Anyway thank you very much for your time.

  0