HTML and CSS only

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted over 3 years ago

  Hi,

  I'm not sure why the project has such a complex set up? You are calling render blocking js in the document head, which doesn't seem to be being used. I recommend you remove that.

  I also think you should take another pass at this challenge tbh. There is a quite a lot of learning points that need addressing before moving on.

  On mobile for me, things are not positioned right and there is loads of scroll in all directions

  I suggest

  • remove excessive divs in the html that aren't needed for this layout
  • semantic html - use meaningful elements for the stats. Numbers don't make sense as headings, and they need to be in the same meaningful element as the word that follows them
  • use css to capitalise text, don't write it like that in html
  • remove all position absolutes and transforms. Create the layout with flexbox. You'll find it easiest if you style mobile first
  • remove large paddings and margins
  • change units in css - font sizes must never be in px, use rem/em; line-height should be unitless like 1.5;
  • remove all the widths and heights. You won't need almost all of them. Just a max-width on the card, min-height on the image, maybe flex-basis 50% on desktop.
  • remove visibility hidden from attribution

  I hope these suggestions help your learning

  2

  ---

  

  Tom•45

  @TdZinkPosted over 3 years ago

  @grace-snow I appreciate your feedback. This will help when I take another pass at it.

  In regards to the font size should you still set the main font-size provided by the guide in px then use rem/em for any changes away from the base?

  0

  

  Tom•45

  @TdZinkPosted over 3 years ago

  @grace-snow I published a new solution being able to address some of the issues you pointed out. I have not yet been able to figure out how to remove all the widths and heights thought I did reduce how many I am using. Same with the paddings and margins.

  0

  

  T

  Grace•31,270

  @grace-snowPosted over 3 years ago

  @TdZink no, font sizes should never be in px. You can change the html/root size with percentages but I wouldn't recommend that either. People with different zoom settings or visual impairments need the root size to be left alone and sizes in rem/em so it scales correctly for them.

  I use a scss function to change px to rem and use it all over the place. It's just pixel-size-you-want / 16 so is pretty straightforward

  1
• 

  James Gascoigne•130

  @Gascoigne09Posted over 3 years ago

  By the looks of it you've tried to use transform to centre your container. You'd need to include this i believe:

  position: absolute;
  top: 50%;
  left: 50%
  transform: translate(-50%, -50%);
  

  2

  ---

  

  Tom•45

  @TdZinkPosted over 3 years ago

  @Gascoigne09 Thank you! This is what I ended up adding to my solution.

  Appreciate the feedback.

  0

  

  T

  Grace•31,270

  @grace-snowPosted over 3 years ago

  @TdZink I wouldn't recommend that technique. Instead make the outer container flex or grid and use their properties to center.

  Eg display grid, place-content center; or display flex, justify content center, align items center

  You may need min-height 100vh on there too

  0

  

  Tom•45

  @TdZinkPosted over 3 years ago

  @grace-snow Thank you for your feedback. I appreciate it.

  0
• 

  Vitor Lacerda•150

  @vitorlfariaPosted over 3 years ago

  Your design looks very good. The only thing remaining is to center it, like @Gascoigne09 said.

  Other thing that you can do is use flexbox on the body. Something like this should work:

  body{
     display: flex;
     align-items: center;
     justify-content: center;
  }
  

  Let me know if this works for you. Keep coding and happy coding! =D

  1

  ---

  

  Tom•45

  @TdZinkPosted over 3 years ago

  @vitorlfaria Thank you! Used grid for the card elements did not think about applying flex from the body to center the card-container.

  Appreciate the feedback.

  1