PhoenixDev22• 16,990
@PhoenixDev22
Posted
Hi Mohammedsalih1,
Congratulation on completing your first frontend mentor challenge. I have some suggestions regarding your solution:
- You should use
<main>landmark to wrap the main body content as HTML5 landmark elements are used to improve navigation experience on your site for users of assistive technology.
- In my opinion, the image is an important content. The alternate text is needed on this image. The alternate text should indicate where the Qr code navigate the user : like
QR code to frontendmentor.ionot describes the image.
- Page should contain
<h1>. In this challenge to tackle the accessibility issue, as it’s supposed to be a part of a whole page, you may use<h1>visually hidden with class=”sr-only”. You can find it here. Then you can use<h2>instead of<h4>. Remember to use the headers in a chronological order.
- In order to center the card on the middle of the page , you can use the flexbox properties and
min-height: 100vhfor the<body>add a little padding to the body that way it stops the card from hitting the edges of the browser. No need for position absolute.
Personally, I don’t restrict the width of the body element. If I need to restrict the width I use a container div with a max-width on it.
width: 250px;an explicit width is not a good way to have responsive layout . Consider usingmax-widthto the card inrem.
- Consider using rem for font size , it' not recommended to use px for font size as absolute units don’t scale for example 15px will always be 15px on the same device. Using pixels is a particularly bad practice for font sizing because it can create some accessibility problems for users with vision impairments.
- Remember a modern css reset on every project that make all browsers display elements the same.
Overall, your solution looks great. Hopefully this feedback helps.
Marked as helpful
0
