NFT preview card component

Community feedback

• 

  Kenneth•240

  @ken0063Posted almost 3 years ago

  Great job!. To fix your accessibility issues you can change

  <div class="container"> to <main>, change all <article> to <div> , change <div class="attribution"> to <footer> and changing <h2> to <h1>. Remember to change the names in your CSS also. To center the page content add {display: grid; place-items: center; width: 100%; height: 100vh;} You can also add width to the styles for main and give h1 transition and font size.

  Marked as helpful

  1

  ---

  

  Amr Elsayed•10

  @AmrSayed74Posted almost 3 years ago

  @ken0063 thank you so much for your feedback ♥ I will try to change some parts of my code

  1
• 

  ieeah•40

  @ieeahPosted almost 3 years ago

  Hi Sayed, first of all good job! Id' like to give you a few advices which would, in my opinion, improve your code :)

  1. You used a lot of different <article> inside of the main container, but single div, containing two different sections would have been a lott easier to style, and also more correct, since everything is part of the same element, and not different elements. For Example:

  <div class="container"> <div class="card"> <div class="cover"> <img class="square" src="" alt="" /> </div> <div class="details"> <h3> title </h3> <h5>subtitle</h5> ecc ecc </div> </div> </div>

  this would also make a lot easier the targeting of the elements in the css, I've seen you used a lot of :nth-of-type, which is really cool that you know, but used like this are a lot of energy and effort which could be used on a better way.

  2. about the <article></article> tag itself, it would'nt be semantically correct: The articole tag is used for shop-articles, or blog-articles; of course you can use it as you prefer, but i think a regular div with appropriates classes would be better, but probably that's just my opinion :p

  3. in the css, from line 119 to line 134, you use a lot of flex, and margins mixed together, but would had been a lot easier to a) not using <ul> and <li> but just using a regular <div> with "display: flex;" and "justify-content: space-between;" to reache the same result (maybe i'm missing some reasonss for which you decided for that ath, if it's the case, sorry :p)

  Anyway, keep working and develop on!

  Marked as helpful

  1

  ---

  

  Amr Elsayed•10

  @AmrSayed74Posted almost 3 years ago

  @ieeah I am so grateful to you thank you so much for your feedback ♥ I will reconsider and change some parts of my code.

  0