Design comparison
Solution retrospective
All feedback will be very much appreciated.
Community feedback
- @0xabdulkhaliqPosted over 1 year ago
Hello there π. Congratulations on successfully completing the challenge! π
- I have other recommendations regarding your code that I believe will be of great interest to you.
COMPONENT MEASUREMENTS π:
- Use
min-height: 100vh
forbody
instead ofheight: 100vh
. Setting theheight: 100vh
may result in the component being cut off on smaller screens.
- For example; if we set
height: 100vh
then thebody
will have100vh
height no matter what. Even if the content spans more than100vh
.
- But if we set
min-height: 100vh
then thebody
will start at100vh
, if the content pushes thebody
beyond100vh
it will continue growing. However if you have content that takes less than100vh
it will still take100vh
in space.
.
I hope you find this helpful π Above all, the solution you submitted is great !
Happy coding!
Marked as helpful1 - @dimar-hanungPosted over 1 year ago
Hi.. π, Congrats completing this challenge πΒ .
I have some interest and feedback with your code
That i like:
- I appreciate the similarity of your results with the design, a bit different in font scale and position, but still good
- HTML is pretty good, not too nested π
- Responsive until galaxy fold screen size π
- CSS Naming is also good, represent what is it for, very clear π
My Feedback:
-
I suggest you use html semantic convention, for exampleΒ
<div class="container">
Β toΒ<main class="container">
, it will make it clearer, and will improve seo if you want to submit your website to google, i recomended this article:Β here -
Maybe you can use tools like prettier to format your code to be more beautiful ( TIP: set prettier configuration to format on save, make it easier )
-
You can seperate file by folder to be more structured, for example:
public/ ββ images/ β ββ qr-code-image.png ββ styles/ β ββ main.css ββ favicon.png index.html
-
To make it center, you can add this is in body style
display: grid; place-items: center;
and remove footer, or move footer to inside container
but overall is good, nice solution, hope useful π
Marked as helpful0@felixicityPosted over 1 year agoThanks, I appreciate you taking out time to review my solution @dimar-hanung
0 - @ecemgoPosted over 1 year ago
Some recommendations regarding your code could be of interest to you.
HTML
In order to fix the accessibility issues:
- You need to replace
<div class="container">
with the<main class="container">
tag and<div class="attribution">
with the<footer class="attribution">
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 page should contain a level-one heading. So, you should use one
<h1>
element in the<main>
tag. You can replace your<p class="content-title">Improve your front-end skills by building projects</p>
element with the<h1 class="content-title">Improve your front-end skills by building projects</h1>
element.
After committing the changes on GitHub and you need to deploy it as a live site. Finally, you should click generate a new report on this solution page to clear the warnings.
CSS
- In order to center the card easily and correctly, you can add flexbox and
min-height: 100vh
to thebody
body{ display: flex; flex-direction: column; justify-content: center; align-items: center; min-height: 100vh; /* height: 100vh; */ }
- After adding them, you don't need to use
margin
in the.container
, you can remove it. - If you want to make the card responsive, you can use
max-width
instead ofwidth
.container { max-width: 300px; /* width:300px; */ /* margin:3rem auto; */ }
Hope I am helpful. :)
0 - You need to replace
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