PhoenixDev22• 16,990
@PhoenixDev22
Posted
Hi Felix Makinda,
Congratulation on completing your first frontend mentor challenge. I have some suggestions regarding your solution:
- 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.
- 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 and the negative margins.
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: 300px;an explicit width is not a good way to have responsive layout . Consider usingmax-widthto the card inrem.
height: 600px;It's not recommended to set fixed height to component, you almost never want to set it. let the content of the component define the height.
- 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
1
Felix Makinda• 10
@felixmakinda
Posted
@PhoenixDev22 Thank you for the kind and insightful words. I will use your suggestions to make improvements. I appreciate your comments on the rem and 100vh.
1
PhoenixDev22• 16,990
@PhoenixDev22
Posted
@felixmakinda Glad to hear that it was helpful. Happy coding!
0
