Stats-preview-card

Community feedback

• 

  T

  Grace•31,090

  @grace-snowPosted 9 months ago

  I hope this feedback is helpful

  1. You don't need multiple reconnect links for Google fonts. You can even download all of the families an weights in one go as well.
  2. You need to think through the html more carefully. Try to imagine the content on its own without any styling. Those 3 stats in the component would be a list with 3 bullets so that tells you what the html needs to be.
  3. Do not mess with roles. There is no counter in this! That's a definite accessibility failure adding incorrect roles to content. Remove it. (And maybe go read about what it is)
  4. Always use a full modern css reset at the start of the styles in every project. Get into the habit now. Andy Bell has a good one you can look up and use.
  5. The mobile styles should be the default. That's another habit you need to get into ASAP.
  6. If this was a real project you wouldn't want to style all em tags like this. Try to approach these single component challenges like you are still building them to go into a bigger site. It would be much better to use a class selector for this kind of styling. You only want to use element selectors for setting sensible defaults at the start of a project.
  7. Media queries must be defined in rem or em not px. This post on media queries and responsiveness is worth reading
  8. Don't set max width on the component to be 70%. That is making this super tiny and narrow on my small phone. For others it will be to wide. Dont use viewport widths either for max width. These units you've chosen remove all control from the layout. Use a sensible rem value for the component's max width — one value for mobile and a wider value for large screens.
  9. Don't set the image height in viewport units either. Again use rem or px. Or At least use a min() or max() function that also gives a non viewport based size. Think about how tiny 30vh is on a mobile landscape view or smaller phone. Way too small.
  10. It would actually be simpler to do the layout and much better for performance to use the picture element in this challenge and not background images. It's also bad practice to have empty divs in html.
  11. There is no need for a pseudo element in this challenge. You are making it harder for yourself.
  12. Never set font size in em. That u it compounds so it's a very dangerous precedent. It can lead to horrible bugs when you lose control of the font size. Font size should be in rem. The em unit is for other properties that you want to scale with the font size. You only une it very intentionally, like for padding on buttons so they grow proportionally when the font size is altered.
  13. Don't use key word values for font size either. You lose all control. The browser a user is on would then make the decisions on what that means.
  14. This should only have one media query. Again, read the linked article above. You should not be using both min and max width queries. Pick mobile first and have a single min width media query defined in rem or em only. Styles should not repeat in media queries either.
  15. Give a wrapping element like body a little padding on all sides or the component some margin on all sides. It is currently hitting screen edges when it should not.

  Marked as helpful

  0
• 

  Anyona Zaddock Ogada•190

  @zacc-anyonaPosted 9 months ago

  Thank you Grace for your feedback.

  I love the way you are straight forward and concise. I really appreciate it, especially as beginner web developer it really comes as helpful.

  I am going to correct those areas you have mentioned, so I don't repeat the same mistakes on my next challenge which I already started. I'm pausing working on it till I familiarize myself with the mistakes and correct them.

  0