Article Preview Solution

Community feedback

• 

  William Richard•10

  @williamrichaardPosted about 4 years ago

  Your work was very beautiful, Joanne.

  To improve your code in the Javascript part, add the "var" keyword to this declaration of "setColor" to make it explicit. Example:

  • var setFlex = new Set();
  • var setColor = new Set();

  Challenge by Frontend Mentor ----->

  Add rel = "noopener noreferrer" to this link to prevent the original page from being modified by the opened link.

  Example:

  • a rel="noopener noreferrer" target="_blank" href="https://www.frontendmentor.io?ref=challenge"

  0

  ---

  

  Joanne•215

  @josnoPosted about 4 years ago

  Thanks! Should've caught my declarations - I code in React alot so I complete forgot it. This proved to be a good review.

  0
• 

  David Payne•1,205

  @dpayne713Posted about 4 years ago

  Nice job! If you have a Mac - check it out on Safari. I had the same issue on mine at first and had to set the inner container height to make it a bit more cross-browser compatible.

  0

  ---

  

  Joanne•215

  @josnoPosted about 4 years ago

  I also had to set the height for other divs and my image container! Thanks for the feedback.

  0
• 

  Justin Punzalan•535

  @justAdevTVPosted about 4 years ago

  @joanne it looks good on my phone and desktop. Nice work :D

  Suggestions for improvement:

  • Add a box-shadow for the card. It's an easy point!
  • Use rem or em for fonts instead of px (check this out for why: https://engageinteractive.co.uk/blog/em-vs-rem-vs-px).
  • At 782 width, the text on the bottom, "Challenge by Frontend Mentor. Coded by Joanne.", is off-center. It's also a bit small. We want to be able to read the footer easily so we know what you got to say!

  Again, great work overall 😄

  -Justin P

  0

  ---

  

  Joanne•215

  @josnoPosted about 4 years ago

  I appreciate your feedback. Thanks for catching the issue with the body at 782 width - I ended up learning more about fit-content!

  1