SASS, PseudoElement, SVG

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  Hi, Benjamin! 👋

  Congratulations on finishing this challenge! 🎉

  Regarding the problems that you had.

  • For the illustration image, it's hard. When I was doing the challenge, it took a lot of time to make position those images correctly. I used the combination of a background image and img elements.
  • I would not recommend specifying any height to the card element. When I inspected the card element and remove the height of the card, the card still looks good. I don't think there's a way to prevent the jumping layout because each content has a different height. So, I think it's okay to let the element move a little bit each time the user opens another accordion panel. (it's just my opinion).

  Now, for the feedback.

  • What's the purpose of the setTimeout? If it's only for development purposes, I highly suggest removing it.
  • I recommend adding more margin-top on the body element. Currently, on my mobile view, the illustration image touches the top edge.
  • I suggest separating the function from the event listener. This way, you can give a descriptive name to the function. By doing this, it allows other developers to understand what the function is doing without actually seeing the lines of code inside it.

  I hope this helps! Keep up the good work! 👍

  Marked as helpful

  2

  ---

  

  Benjamin•130

  @BenjoowPosted over 2 years ago

  @vanzasetia Thank you a lot for your precious feedback !

  I'm modified my solution by removing setTimeout (i forgot to delete it) and creating a new media query to make things right on your mobile device. (i hope it's work). Even for such simple exercice, you can have so many media query...

  How do you manage that ? Any advice or article to read ?

  Oh and i kept the height of my card, i don't like to see the illustration jumping when i deploy my accordion :-).

  Once again, thank you Vanza !

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @Benjoow I think this only needs one media query for the desktop layout. So, to simplify the styling,

  • First, style the mobile layout
  • Second, style the desktop layout with a media query
  • That's it! 🙂

  I believe it's about practice because I think practice makes me better. So, don't worry too much about it, you can always come back and refactor the CSS once you've gained more experience.

  You're welcome, Benjamin! 👍

  0