QR Component using HTML and CSS (2nd iteration)

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted about 1 year ago

  For CSS I'm going to give some essential tips and principles for you to have a go at refactoring rather than going through line by line

  1. Always use a modern css reset at the start of the styles. Andy Bell has a good one you can look up and use. Be sure to read his post(s) about it properly to understand what it is and why it includes what it does
  2. Never declare font sizes in px
  3. Remove all heights and widths in this (min height is fine on the body). Never limit the height of elements that contain text. As developers we have to build robust solutions that could consume content of a different length if editors change it, or different default font sizes which many many people change
  4. Most components, this includes, only need a max-width in rem, no widths
  5. Correct colors are provided in the style guide. The text must be readable
  6. Not everything needs to be flex. Only use flex very intentionally
  7. Line height should be set with a unitless value eg 1.5. Don't rely on keywords that can differ between browsers.
  8. Text align is inherited if placed on elements higher in the DOM
  9. give the component a little margin or the body a little padding to prevent the component from ever hitting screen edges
  10. There is no need for any media query on this challenge. You will need them on future challenges so it's worth reading about how to use media queries well

  Marked as helpful

  1

  ---

  

  T

  Grace•31,270

  @grace-snowPosted about 1 year ago

  This is much better now

  You've still not Included the modern css reset. Although that isn't causing problems in this specific challenge it will cause problems in future challenges. No real-world project would miss a reset so get into the habit of adding it now

  Reminder:

  Always use a modern css reset at the start of the styles. Andy Bell has a good one you can look up and use. Be sure to read his post(s) about it properly to understand what it is and why it includes what it does

  You're also using custom properties in an unusual way here. Custom properties are overwritable, they are designed to be changed in some contexts, ie values changed for certain components or values changed site wide based off a user action like toggling a theme change. You would naver name custom properties with values within them like "--margin-bottom-10px". A more usual name would be something like --spacer-small

  You are also setting margin bottom on elements to 0 on line 27 (unnecessary anyway) then setting it again on lines 67 and 74. This is confusing.

  The last warning is about using element selectors. When coding single components like this, still think about a whole website. Because you've used element selectors for paragraph and the heading, these styles would affect everywhere in the site. It is good practice to only use single class selectors in css after the Reset for component styles. This ensures they only affect the components you want to, and it keeps css specificity low

  1
• 

  T

  Grace•31,270

  @grace-snowPosted about 1 year ago

  This looks great now. I don't think you need the .container div at all though. All of those styles can go on the body:

  display: flex;
    align-items: center;
    justify-content: center;
    flex-direction: column;
    min-height: 100vh;
    text-align: center;
  }
  

  Marked as helpful

  0

  ---

  

  Priyanka Roy Choudhury•100

  @PriyankaRC16Posted about 1 year ago

  @grace-snow thank you!

  0
• 

  T

  Grace•31,270

  @grace-snowPosted about 1 year ago

  First things first, you need to completely rewrite the html on this I'm afraid

  • Indent your code consistently so it's readable and easier to spot bugs. Your code editor can even do this formatting automatically for you
  • always use landmarks to contain content. This should have a main (Every page should) and a footer just for the attribution
  • never have text in divs or spans alone. Think about the meaning of the content and choose appropriate meaningful elements. This component clearly has an image, a heading and a paragraph. As you move onto other challenges there will be other elements required. Take time to think that through
  • don't use inline styles (there may be exceptions to this, but they are a long long way in the future - for now, treat that as a rule)
  • the image is the most important content in this component. Yet you have given it a description of "no image". It needs a properly written description saying what it is (QR code) and where it goes (to FrontendMentor.io)
  • do not split elements to force line breaks in specific places. Line breaks will occur naturally based off the available space and any max-widths applied
  • load fonts in the html head. Google fonts will give you the exact code to use once you select the fonts and weights you need

  Marked as helpful

  0
• 

  P

  gladstone28•530

  @gladstone28Posted about 1 year ago

  I like your solution to the QR Code component challenge. However I notice the following error: In the HTML, you are missing the closing </div> tag for the <div class="body-card"> element.

  Marked as helpful

  0

  ---

  

  Priyanka Roy Choudhury•100

  @PriyankaRC16Posted about 1 year ago

  @gladstone28 thanks a lot! Just fixed it

  0
• 

  P

  Øystein Håberg•10,710

  @Islandstone89Posted about 1 year ago

  You should not have a div outside main. Change .container to main, and .card to div.

  0
• 

  lisztomania23•170

  @lisztomania23Posted about 1 year ago

  Your solution seems fine, but you did not use the 'Outfit' font. Also, why the individual div for every text line, two would have been sufficient. Consider using relative properties instead of fixed for better responsiveness like min-width and max-width. Could you reduce the font size for the attribution and fix it to the bottom, or place it as preferred.

  0