Basic NTF Preview-card

Community feedback

• 

  Abdul•8,540

  @SamadeenPosted over 2 years ago

  Hey!! Cheers 🥂 on completing this challenge.. .

  Here are my suggestions..

  • You should use <main class="card-body"> instead of <div class="card-body">.
  • You can wrap your attribution section in a footer tag to avoid accessibility issues.

  This should fix most of your accessibility issues

  . Regardless you did amazing... hope you find this useful... Happy coding!!!

  Marked as helpful

  1

  ---

  

  Keagan Dickinson•160

  @PhisherFTWPosted over 2 years ago

  @Samadeen Hi Abdul, thank you for your suggestions! I have since implemented them and I will be trying my best to keep my code clean and with good accessibility in the future.

  0
• 

  P

  Grog the Frog•480

  @GregLyonsPosted over 2 years ago

  Good work!

  I actually think your solution to the tricky image-hovering problem is pretty good. When you say you're not entirely sure how it works, is it the CSS that you wrote that you're having trouble understanding? The hover effect is achieved using the :hover selector. The key is your two rules:

  .container:hover .image-equil {
  opacity: 0.3;
  }
  
  .container:hover .middle {
  opacity: 1;
  }
  

  The rules are stating the following:

  1. When .container is being hovered, reduce the opacity of any of its .image-equil descendants.
  2. When .container is being hovered, increase the opacity of .middle (which is otherwise at 0) to 1.

  I believe what's happening here is this: the background of .container is that teal color, so when you reduce the opacity of .image-equil--which is taking up all of .container, the background color shows through (Rule 1). Then, the content of .middle consists of the eye image (which you've centered using CSS), and when hovering the eye becomes fully opaque, and actually blocks the portion of the .image-equil that is under the eye image (Rule 2). This is the desired effect, as you don't want to see the NFT image through the white eye (according to the design).

  I don't think it's necessary to have the .middle <div>, as you already have the .middle class on the eye <img> itself, so even without the <div> it'll still be a descendant of .container, and Rule 2 will apply to it. Other than that, I don't see much way to simplify your image hovering thing.

  Two more things I want to point out:

  1) For the author name "Jules Wyvern", you have a separate <p>, which you set to display: inline; so that it'll be on the same line as "Creation of". There's already a specific HTML element for what you're trying to do there, however: the <span>. Using a <span>, you'd have

  <p class="creator">Creation of <span class="name">Jules Wyvern</span></p>
  

  Since <span>s are inline elements, there's no need to change the display property in that case. This is also better since, if you think about the problem in terms of what the HTML elements you have are saying, they're indicating that "Creation of" and "Jules Wyvern" are in separate paragraphs, which is not what you're trying to convey. Instead, you want "Creation of Jules Wyvern" to be a single paragraph <p>, and then you can change text within that paragraph using a <span>.

  2) When you feel ready, I recommend learning Flexbox as soon as you can. It'll make solving these UI/layout challenges a lot easier. For example, your CSS:

  .eth-txt {
      position: relative;
      display: inline;
      width: 2rem;
      padding-right: 3.25rem;
      color: hsl(178, 100%, 50%);
  }
  .days-left {
      display: inline;
      position: relative;
      width: 2rem;
      color: hsl(215, 51%, 70%);
  }
  

  which you use to position the ETH text and days left, only works for the very specific width of the box. If the width of the .card-body element were to change at all, suddenly your padding-right on the .eth-text would no longer make it look like the two pieces of text are on opposite sides of the same line.

  A more robust (i.e. works at any screen size/container size) would be to put your .eth-text and .days-left in a wrapper <div class="wrapper"> with

  /* Selects the .wrapper <div> */
  .wrapper {
    /* Turns .wrapper into a Flex container */
    display: flex;
  
    /* Direct children of .wrapper placed on opposite sides/spaced out evenly up to the edges */
    justify-content: space-between;
  }
  

  (Note that you'd need to put the ETH icon in the same block as .eth-text to make this work--but this would be the solution without the icon.)

  I hope this helps. If so, feel free to mark this comment as helpful. Either way, best of luck in your future projects!

  2

  ---

  

  Keagan Dickinson•160

  @PhisherFTWPosted over 2 years ago

  @GregLyons Hey Greg, thank you so much for your feedback, that explanation of my CSS hover code is exactly what I needed thanks! I have implemented your suggestions and improvements 1)Removed the excess <div> with the class middle as it was not needed 2) Added <span>'s instead of modifying <p> elements to be inline 3) Added flexbox with the wrapper class, and made it more reactive (flexbox looks awesome)

  Here is my updated version (if you have time to take a look): Repository: https://github.com/PhisherFTW/NFT-Preview-card Live Version: https://phisherftw.github.io/NFT-Preview-card/

  0