Design comparison
Solution retrospective
All kind of Feedbacks are welcome. Thank You!
Community feedback
- @NaveenGumastePosted almost 3 years ago
Hello Sachin Mulgir ! Congo π on completing this challenge
Let's look at some of your issues, shall we:
- Warp your card content around the main tag Ex: π
<body> <main class="container"> *all you content here* </main> </body>
happy Codingπ
Marked as helpful0 - @GitHub-dev12345Posted almost 3 years ago
Congratulation ππ complete your challenge. Used this code want your card in center position :
in body tag Used this CSS Code: body{ display : flex; justify-content: center; align-item: center; }
in Card Design CSS Code used this property: align-self: center;
And
β€οΈπ My small suggestion : 1.In Card design CSS Code Used this:
transform : scale(0.8); this property decrease the size of card. π
large size for increase the number of scale & small size for decrease the number of scale
I hope you find this helpful
Marked as helpful0 - @NaveenGumastePosted almost 3 years ago
Hello Sachin Mulgir ! Congo π on completing this challenge
Let's look at some of your issues, shall we:
-
Always use
h1
first and thenh2
,h3
and so on -
To center the card vertically
max-height: 100vh; display: flex; justify-content: center align-item: center;
happy Codingπ
0 -
- @PhoenixDev22Posted almost 3 years ago
Greeting @SachinMulgir ,
Congratulation on completing your first Frontend mentor challenge,
I have few suggestions regarding your solution:
-
Page should contain a level-one heading . As this is not a real webpage , you can use
<h1>
withclass="sr-only"
(Hidden visually, but present for assistive tech) -
Use the main landmark to wrap the body content which is the card .HTML5 landmark elements are used to improve navigation .
-
To center the card on the middle of the page , you can use the flexbox properties and
min-height: 100vh
for the<body>
.Remove the margin from the card
body { text-align: center; font-family: 'Outfit', sans-serif; background-color: #d6e2f0; display: flex; align-items: center; justify-content: center; min-height: 100vh; }
-
Using percentages for the width , Not a great thing as you're losing control of the layout. Use
max-width:20rem
instead, let it grow to a point. Then a little margin on the component or padding on the body to stop it hitting screen edges. -
It's recommended to use
em
andrem
units .Bothem
andrem
are relative units , Usingpx
won't allow the user to control the font size based on their needs.
Overall , your solution is good. Hopefully this feedback helps.
0@SachinMulgirPosted almost 3 years ago@PhoenixDev22 Thank you for the feedback. I will try to work on the mentioned parts.
0 -
- @denieldenPosted almost 3 years ago
Hi Sachin, I took some time to look at your solution and you did a great job!
Also I have some tips for improving your code:
- add
main
tag and wrap the card for Accessibility - use
h1
tag for the title and notp
element - To make it look as close to the design as possible set
width: 20rem;
to.qrBox
class - remove all
margin
from.qrBox
class - try to use flexbox to the body for center the card. Read here -> best flex guide
- after, add
min-heigth: 100vh
to body because Flexbox aligns child items to the size of the parent container
Overall you did well :)
Hope this help and happy coding!
0@SachinMulgirPosted almost 3 years ago@denielden Thank You for the feedback and suggestions. Will try to work on those parts.
1 - add
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