Responsive layout for the QR code challenge

Community feedback

• 

  Daniel 🛸•44,250

  @danielmrz-devPosted 8 months ago

  Hello @Nomylim!

  Your solution looks great!

  I have a couple of suggestions (about semantic HTML) for improvement:

  📌 First: Use <main> to wrap the main content instead of <div>.

  Think of <div> and <span> in HTML like plain boxes or placeholders. They're handy for holding content, but they don't tell us anything about what's inside or what it's meant for on the webpage.

  📌 Second: Use <h1> to <h6> for titles instead of <p>.

  The tag <p> is meant for paragraphs. For titles, we have the HTML headings (the tags <h1> to <h6>).

  Here's a quick guide on how to use them:

  Unlike what most people think, it's not just about the size and weight of the text.

  • The <h1> to <h6> tags are used to define HTML headings.
  • <h1> defines the most important heading.
  • <h6> defines the least important heading.
  • Only use one <h1> per page - this should represent the main heading/title for the whole page. And don't skip heading levels - start with <h1>, then use <h2>, and so on.

  All these tag changes may have little or any visual impact but they make your HTML code more semantic and improve SEO optimization as well as the accessibility of your project.

  I hope it helps!

  Other than that, great job!

  Marked as helpful

  1
• 

  P

  Øystein Håberg•10,390

  @Islandstone89Posted 8 months ago

  Hello, Nomi. Well done completing your first project!

  Here are some suggestions that hopefully will help you :)

  HTML:

  • Every webpage needs a <main> that wraps all of the content, except for <header> and footer>. This is vital for accessibility, as it helps screen readers identify the "main" section of a page. Change the first <div> to a <main>.

  • It is common to use classes instead of IDs.

  • The alt text must also say where it leads(frontendmentor.io).

  • "Improve your front-end skills by building projects" is a heading. Change it to a <h1>.

  • .attribution should be a <footer>, and its text must be wrapped in a <p>.

  CSS:

  • It's good practice to include a CSS Reset at the top.

  • Do not use % for border, margin or padding, and usually not for widths either.

  • Remove border and margin on the body. Change padding to 1rem, so the card doesn't touch the edges on small screens.

  • Remember to declare a fallback font, in case the user can't access the specified font:font-family: "Outfit", sans-serif;.

  • On the body, add min-height: 100svh;. This will make the body take up atleast the whole height of the viewport, while still being allowed to grow (setting a height would cause overflow if the content were to grow beneath the height). The body is by default only as tall as its content. This might be a bit confusing, since the background-color we give it fills the whole viewport. But if you were to give body a border, you would see that it doesn't grow taller than the content inside it. It does fill the entire width of the viewport by default, so there's an important difference to take note of.

  • Also on body, add gap: 2rem; to create some space between the <main> and the <footer>.

  • Remove the border on the card.

  • padding on the card should be equal on all 4 sides. I would also increase it to around 10px.

  • Remove the width and height on the card. Whether it's in % or px, setting fixed dimensions is generally a no-no. The problem with setting fixed sizes is that the web is not a fixed medium. Components must adapt to all kinds of screens, and they cannot do that with a fixed size. If the viewport happens to be smaller than the fixed size, it will cause overflow, meaning you would have to scroll horizontally to see all of the content. This is bad user experience, and it also looks terrible.

  • Add a max-width of around 20rem on the card, to prevent it from getting too wide on larger screens.

  • font-size must never be in px. This is a big accessibility issue, as it prevents the font size from scaling with the user's default setting in the browser. Use rem instead.

  • Since all of the text should be centered, you only need to set text-align: center on the body, and remove it elsewhere. The children will inherit the value.

  • Paragraphs have a default value of font-weight: 400, so there is no need to declare it.

  • Remove everything on the image, except for border-radius. Add display: block and max-width: 100% - the max-width prevents it from overflowing its container.

  • As the design doesn't change, there is no need for any media queries. When you do need them, they should be in rem, not px. Also, it is common practice to do mobile styles first and use media queries for larger screens.

  Marked as helpful

  1

  ---

  

  Nomy•130

  @NomylimPosted 8 months ago

  Hi @Islandstone89

  Thank you so much for your feedback; it's really helpful. I'm currently making changes to my code and I happen to have a question. When you say that 'The alt text must also indicate where it leads (frontendmentor.io),' are you referring to the image properties or something else?

  1

  

  P

  Øystein Håberg•10,390

  @Islandstone89Posted 8 months ago

  @Nomy you simply have to add a few more words in the alt attribute on the image: <img src="images/image-qr-code.png" alt="qr code that leads to the Frontend Mentor website." />

  Marked as helpful

  0