@gmagnenat
Posted
Hi, congrats on completing the challenge !
I noticed a few issues on your code that you may want to check. These are recommandations to improve your solution and good habits to take. Please check them out and reach out to me on discord if you have any doubt or questions about them.
Does the solution include semantic HTML?
- the minimal <main> landmark is here and properly used. You are also using the footer landmark for the attributions.
- You could specify a better alt for the image. It should describe well what it is in case of this image cannot be seen. Here you can specify for example the target where this qr code will lead.
- A performance recommandation : move the title in the head higher just under the
<meta name=viewport...
. There is great ressources about this shared on discord. You want the title of the page to displayed before everything else when a page is loading.
Is it accessible, and what improvements could be made?
- Currently you have an accessibility issue mostly caused by improper use of font-sizes, fixed width and height set on elements containing text. Users with visual impairement or low vision caused by disabilities or just aging will increase the default font size of the browser. If you are using pixels for your font sizes you set the size at a fixed value that will not respect the user settings and will not change accordingly. You need to use relative units for everything related to fonts (rem).
- If you set a fixed width in pixels it's the same issue. The content will not scale if the font-size is increased so it will cause overflow and your layout will break. Use
min-width
in rem. - Don't set any fixed height on element that are containing text. Leave the content flow and scale naturally. If you set a fixed height you will again have overflow problem and a bad user experience for some people.
Is the code well-structured, readable, and reusable?
- In addition to the point listed above, you have some useless rules in your stylesheet.
- Remove the height on qr-box
- Remove the height on qr__img and remove the width. It is contained in qr-box. just use a padding on qr-box
- remove the width on qr__title and qr__undertitle. It's also contained in qr-box so you don't need to specfiy a width. You can limit the width of the text using the
ch
unit if you want. - remove the large padding-top on the body and use flexbox on your body to center the content.
Does the solution differ considerably from the design?
- the live url does not work.
I hope you find some useful comments in this list. Sorry for the long list but I like commenting this challenge as I think its a perfect step to identify some pitfalls in fundamentals before moving to more complex projects. Reach out to me on discord if you have any questions about the list.
Happy Coding !
Marked as helpful