QR Code Page - HTML & CSS

Community feedback

• 

  P

  visualdennis•8,375

  @visualdennissPosted over 1 year ago

  Hello there,

  your card looks pretty good. It is nice that you have already implemented the other suggestions about centering etc.

  • Additionally, i'd say try to avoid giving fixed heights like this: height: 497px; use only min-height when necessary, but most of the time you don't need any height. Height should be decided by the content of the container, if needed simply tweak it with paddings and margins of the contents. Fixed heights are known to cause various issues, resulting in overflowing content when data or font-sizes change.

  • Relatedly try to avoid using px, instead use the responsive rem/em units. Here is a great resource on YT for clarifying all the differences between rem/em and explain why to use them: https://www.youtube.com/watch?v=dHbYcAncAgQ

  Hope you find this feedback helpful!

  Marked as helpful

  1

  ---

  

  Adam•10

  @avgrimshawPosted over 1 year ago

  Applied the suggested changes 🙌

  Thank you for the feedback and advice, it makes a lot of sense, also thanks for the informational YouTube link 😁

  1
• 

  Ecem Gokdogan•9,380

  @ecemgoPosted over 1 year ago

  Some recommendations regarding your code that could be of interest to you. 🤗

  In order to fix the accessibility issues:

  • You need to replace <div> (outside of the <div class="qr-code_container"> ) with the <main> tag.
  • However, you need to keep the <div class="challenge-container"> outside of the <main> tag and after doing it, please replace it with <footer> tag.
  • You'd better use Semantic HTML, and you can also reach more information about it from [Using Semantic HTML Tags Correctly]
  • Each main content needs to start with an h1 element. Your accessibility report states that the page should contain a level-one heading. So, you need to use a <h1> element in the <main> tag instead of using the <h2> element. You can replace the <h2>Improve your front-end skills by building projects</h2> element with the <h1>Improve your front-end skills by building projects</h1> element.

  Hope I am helpful. :)

  Marked as helpful

  1

  ---

  

  Adam•10

  @avgrimshawPosted over 1 year ago

  Hi!

  Thank you for your recommendations on fixing accessibility issues. When I was first starting the project, I did initially use <main> and <footer> elements, but opted for making both parts of the page in a <div> to fix the qr-code_container and challenge-container not appearing vertically inline with each other. Also, I was using <h2> instead of <h1> thinking that it would make replicating the design text easier, but as I've changed it to be semantically correct, I've realised it has no affect on the CSS styling.

  I have changed the respective <div> elements to their semantically correct types and placed the <footer> outside of the <main> element, but of course stumbled across the same alignment issues as before. I quickly realised that I just needed to flex-direction: column; to the body selector and all works perfectly now.

  Once I made the changes I stumbled across an issue with the page being longer and the scroll bar appearing, which was quickly fixed by using the all elements selector * and applying box-sizing: border-box; to it, as well as applying margin: 0; and padding: 0; to html, body selectors.

  Your feedback on fixing the accessibility issues has reminded me that I should first of all make sure that my webpages are accessible through screen readers by using proper Semantic HTML.

  Thank you very much for your feedback, I've learnt something today! 😁

  1

  

  Ecem Gokdogan•9,380

  @ecemgoPosted over 1 year ago

  @avgrimshaw Some feedback is getting instructive. I also learn by researching. It is a pleasure to share some of what I have learned with you and other users here. I'm happy to help :)

  1
• 

  Gio Cura•650

  @GioCuraPosted over 1 year ago

  Hi! 👋 It's a very clever thing for you to incrementally add margin-top via media queries. However, all you need now to add to your code is min-height: 100vh to your <body>. You've already given it display: flex, justify-content: center, and align-items: center. Those four attributes will center the code no matter how wide the screen would get.

  Once you add min-height: 100vh, take out all your margin-top media queries!

  Hope this helps!

  Marked as helpful

  1

  ---

  

  Adam•10

  @avgrimshawPosted over 1 year ago

  Hi! Thank you very much for the useful feedback, just applied it and works perfectly 🙌

  2