Design comparison
Solution retrospective
Can you point out the errors I have made in this project? Solutions and feedback are welcome.
Community feedback
- @MelvinAguilarPosted over 1 year ago
Hello there π. Good job on completing the challenge !
The solution looks much better now, I just have a few small suggestions.
HTML π·οΈ:
- Since this component involves scanning the QR code, the image is not a decoration, so it must have an
alt
attribute. Thealt
attribute should explain its purpose. e.g.QR code to frontendmentor.io
CSS π¨:
- Use
min-height: 100vh
instead ofheight
. Setting the height to 100vh may result in the component being cut off on smaller screens, such as a mobile phone in landscape orientation.
-
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 π.Here is an image of how it would look on a mobile device (taking into account the scroll): screenshot-imgur πΈ
Result using grid layout:
.outer-frame { background-color: hsl(212, 45%, 89%); /* position: relative; */ /* height: 100vh; */ /* width: 100vw; */ /* margin: 0; */ min-height: 100vh; display: grid; place-content: center; } .inner-frame { background-color: hsl(0, 0%, 100%); /* position: absolute; */ min-height: 428px; max-width: 275px; /* top: 50%; */ /* left: 50%; */ border-radius: 17px; /* transform: translate(-50%, -50%); */ text-align: center; }
I hope you find it useful! π Above all, the solution you submitted is great!
Happy coding!
Marked as helpful0 - Since this component involves scanning the QR code, the image is not a decoration, so it must have an
- @frank-itachiPosted over 1 year ago
Hello there π. You did a good job!
I have some suggestions about your code that might interest you.
HTML π:
- Wrap the page's whole main content in the
<main>
tag. - Wrap the
<div class="bottom-frame">
inside the<footer>
tag. - The heading order is important in the html structure so try to always start your headings and/or titles with an
<h1>
tag and then you can increase by one if you need to use more headings in your html code. So replace the<h3>
by the<h1>
tag
I hope you find it useful! ππ Above all, the solution you submitted is greatπ!
Happy
<coding />
π!Marked as helpful0@AlabasterRigPosted over 1 year ago@frank-itachi Thank you for your Feedback! I will make the changes ASAP! I had issues with the <h1> tag being too big for the frame hence I chose the <h3> tag. I will try to look for a workaround.
Thank you!
0 - Wrap the page's whole main content in the
- @kxtaraPosted over 1 year ago
Looks good! Have you tried changing the first
h3
tag to anh1
for accessibility?Happy Coding
0@AlabasterRigPosted over 1 year ago@kxtara Hi! Thank you!. I did try changing the <h3> tag to <h1> but it seemed oddly too big for the frame. I Could have increased the frame size but that would go against the design of the layout. But I think I will be giving it a try and see how it goes.
0@kxtaraPosted over 1 year ago@AlabasterRig I would try changing the font size on the
h1
tagMarked as helpful0
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord