Qr Code Using CSS & HTML5

Community feedback

• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted over 1 year ago

  Hello there 👋. Good job on completing the challenge !

  I have some suggestions about your code that might interest you.

  • I don't see any negative margin values and I believe they are not necessary.

  • You can use the tags you need, just avoid skipping headings elements, start with <h1> and not with <h3>, and you can achieve the same result using font-size in the CSS code.

  • You can upload the image to your Github repository, the challenge is not too big and it will not affect performance. Also, by using an external image, you are making an extra request.

  • The numbers 375px and 1440px are the dimensions of the images, you don't have to set the width of the body to those values. Otherwise, on 4k screens, your element will not be centered.

  • You should not use inline-CSS (CSS in the html code) because it is not a good practice. Instead, you should use an external stylesheet to style your page. By doing this, you will be able to have a better organization of your code and will be able to understand it better.

  • I think you have to upload the images folder and the favicon to your repository.

  • There are too many <br> tags, you are cluttering the HTML code a lot, you can achieve the same result using margins.

  • Avoid using position: absolute to center an element as it may result in overflow on some screen sizes. Instead, utilize the flexbox or grid layout for centering. Get more insights on centering in CSS here here 📘.

  I hope you find it useful! 😄

  Happy coding!

  Marked as helpful

  2

  ---

  

  Angela Moore•160

  @Cyber-ChicPosted over 1 year ago

  @MelvinAguilar

  This is wonderful feedback!! You cleared up a lot of the initial confusion I had when creating this. I will start implementing these changes to my site. Thank you so much for the great responses to my questions - Very helpful!

  I really appreciate your help with this! 😄

  Happy coding!

  0
• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 1 year ago

  Hi, Angela Moore!

  About your questions:

  1. I don't see any margin property in the index.html.
  2. Don't use heading tags for styling purposes! Heading tags are used to structure the page. Meaning, they are meaningful elements. You should use <h1> and adjust the size of the text using CSS. Dive deeper — How-to: Accessible heading structure - The A11Y Project
  3. It's absolutely fine to put the QR code image in your GitHub repository. In fact, you should do that instead of separately hosting the image. This way, you can keep track of the image easily.
  4. Use the .gitignore that comes from the starter files. Then, move it to the project folder on your local machine. Then, upload the file using git command.
  5. That's hard—or even impossible—to know whether you have been following best practices. Also, what do you mean by "best practices"? I recommend for getting started, making sure your code has no issues, is valid, and does a Lighthouse audit.
  6. The sizes on the style-guide.md have nothing to do with the media queries. They are telling you that "this is how your website should look like at these screen sizes". As a front-end developer, you should make your website looks good at all screen sizes, including smaller, larger, and everything in between.
  7. I recommend uploading everything that you get from the starter files.
  8. It's up to you to create a new CSS folder or not, at least for this project.
  9. I recommend using a code-formatter. This way, your code base will have a consistent format, which makes it easier to read the code. I suggest using Prettier as your code-formatter. My recommendation — Prettier · Opinionated Code Formatter

  Some feedback:

  • Remove all <br> elements. Don't use them to create spacing between elements. Screen readers may announce them as "break" which creates unnecessary noises. Use CSS instead. Learn more — <br>: The Line Break element - HTML: HyperText Markup Language | MDN #accessibility_concerns
  • Remove width, min-width, and max-width from the <body> styling. It should always fill the entire page. Treat it as the main element of the web page.
  • Use flexbox or grid to place the card in the center of the page. These modern techniques are more robust than absolute positioning and have less code to write.

  I hope you find this useful.

  Marked as helpful

  1

  ---

  

  Angela Moore•160

  @Cyber-ChicPosted over 1 year ago

  @vanzasetia

  Hi, Vanza Setia!

  This is excellent feedback. Thank you so much for addressing all of my questions and providing additional resources. I especially like the Prettier · Opinionated Code Formatter! Also, I've heard there's an informal set of coding best practices / standards. Your suggestion to make sure there are no issues, that everything is valid, and to do a Lighthouse audit sounds like a wonderful course of action. I will start implementing your changes to my site. Thank you so much for the extremely helpful critique.

  I really appreciate all your help! 😄

  Happy coding!

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 1 year ago

  @Cyber-Chic

  You are welcome! Happy coding too! 👍

  1
• 

  Abdul Khaliq 🚀•72,580

  @0xabdulkhaliqPosted over 1 year ago

  Hello there 👋. Congratulations on successfully completing the challenge! 🎉

  • I have other recommendations regarding your code that I believe will be of great interest to you.

  HTML 🏷️:

  • This solution generates accessibility error reports due to non-semantic markup

  ---

  • So fix it by replacing the <div class="image"> with semantic element <main> to improve accessibility and organization of your page.

  ---

  • What is meant by landmark elements ?, They used to define major sections of your page instead of relying on generic elements like <div> or <span>

  ---

  • They convey the structure of your page. For example, the <main> element should include all content directly related to the page's main idea, so there should only be one per page

  ---

  HEADINGS :

  • Every site must want at least one h1 element identifying and describing the main content of the page.

  ---

  • An h1 heading provides an important navigation point for users of assistive technologies, allowing them to easily find the main content of the page.

  ---

  • So we want to add a level-one heading to improve accessibility by reading aloud the heading by screen readers, you can achieve this by adding a sr-only class to hide it from visual users (it will be useful for visually impaired users)

  ---

  IMAGE :

  • Since this component involves scanning the QR code, the image is not a decoration, so it must have an alt attribute. The alt attribute should explain its purpose. e.g. alt="QR code to frontendmentor.io"

  ---

  I hope you find it helpful ! 😄 Above all, the solution you submitted is great

  Happy coding!

  Marked as helpful

  1

  ---

  

  Angela Moore•160

  @Cyber-ChicPosted over 1 year ago

  @0xAbdulKhalid

  Thank you so much for the detailed response!! This is a fantastic breakdown and extremely helpful. I'm going to work on implementing these changes into my site!

  I really appreciate all your help! 😄

  Happy coding!

  0