Design comparison
Solution retrospective
当時はcssやclassの記法がまだまだわかっていなかったところなどがあるため、それらを変えていきたい。
What challenges did you encounter, and how did you overcome them?cssの書き方ですね。
メンバーに意見をもらったりして、修正しました。
What specific areas of your project would you like help with?モダンな書き方を知りたいと思っております。
例えば、クラス名の書き方、cssの読みやすい描きからなど
Community feedback
- @Islandstone89Posted 12 months ago
Hi, here is some feedback.
HTML:
-
Good job on including the
<main>
, most beginners are not aware of its importance. However, you have way too many divs. In such a simple component, you only need a<main>
, which can also act as the card. So, I would remove all divs. -
The image must have alt text. This is essential for screen readers to understand the image. The alt text should be descriptive, and in this example, it also needs to say where it leads (frontendmentor.io).
-
"Scan the QR code" and the footer text should both be wrapped in a
<p>
. -
Hence, you should end up with the following HTML structure:
| <body> || <main> ||| <img> <h1> <p> || </main> || < footer> ||| <p> || </footer>
CSS:
-
Performance-wise, it's better to link fonts in the
<head>
of the HTML then using@import
. -
It's good practice to include a CSS Reset at the top.
-
Remove
width: 100%
onhtml
,main
andbody
- they are 100% wide by default, because they are block elements. -
Change
height: 100vh
tomin-height: 100vh
. And you only need to put it on thebody
. -
Background color and text alignment should be moved from the card to the
body
. Additionally, add Flexbox to center the card horizontally and vertically:
background-color: hsl(212, 45%, 89%); text-align: center: display: flex; flex-direction: column; justify-content: center; align-items: center; min-height: 100vh;
-
max-width
on the card is way too big, and should not be in px but in rem. 1440px equals to 90 rem, but around 20rem should be all you need. -
Remove
width: min-content
. -
Font-size must never be in px. Use rem instead.
-
Remove the fixed width and height on the image. You rarely want to set fixed dimensions, as this easily creates issues with responsiveness.
-
Do add
display: block
andmax-width: 100%
on the image. The max-width prevents the image from overflowing the card.
Marked as helpful1@amakura-411Posted 12 months ago@Islandstone89
Thank you for the clear feedback, I appreciate it!
I've improved the code based on your advice.
However, there's one thing I'm not clear about.
You suggested using max-width in rem instead of px. You mentioned that 1440px equals
90rem
, but recommended using20rem
. How did you determine max-width: 20rem?I want to know how you arrived at that. (It might be adjustments based on actual screen view, but...)
I'd appreciate it if you could provide an answer.
Thank you in advance for your feedback!
1@Islandstone89Posted 12 months ago@amakura-411 Happy to help :)
If you open DevTools (click F12 in Chrome), you can select "Responsive" in the list of simulated devices. Then, you can adjust the screen size smoothly, and see where the card starts to get too big.
Marked as helpful1@amakura-411Posted 12 months ago@Islandstone89
Thank you for your response!
Thanks to you, it looks like I'll be able to write some good code!!
I'm grateful to have met someone as kind as you! :))
1 -
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