Design comparison
SolutionDesign
Solution retrospective
Looking for advice on how to the the margins rights.
Also, the line break doesn't fully match the design, any advice?
Thanks!
Community feedback
- @MuzzammmillPosted about 2 years ago
Hi yolanda1089,
Congratulation on completing this challenge. Your solution looks great. I have some suggestions regarding your solution if you don’t mind:
- Giving the columns a width of 30% and the container (section) a min-width and the max-width because it affecting your media query #sedan-column, #suv-column, #luxury-column { width: 30%; /* width: 307px; */ height: 500px; color:hsla(0, 0%, 100%, 0.75); padding-top: 50px;; padding-left: 50px; padding-right: 65px; box-sizing: border-box; min-width: 800 } If you adjust your screen size to 521px and 918px you will see what i am talking about.
0 - @PhoenixDev22Posted about 2 years ago
Hello yolanda1089,
Excellent work! I have some suggestions regarding your solution:
- Use the
<footer>landmark for the attribution, as using landmarks is important to improve navigation experience on your site for users of assistive technology.
- About
<h1>it is recommended not to have more than one h1 on the page. Multiple<h1>tags make using screen readers more difficult, decreasing your site’s accessibility. In this challenge , as it’s not a whole page, you can have<h1>visually hidden withsr-only. Then you can swap those<h1>with<h2>.
- In this challenge, the svgs are much likely to be decorative. For any decorative svgs, each svg tag should have
aria-hidden="true"andfocusable=”false”attributes to make all web assistive technologies such as screen reader ignore those svgs .
- Never use
<div>and<span>alone to wrap a meaningful content. Just keep in mind that you should usually use semantic HTML in place of the div tag unless none of them (the semantic tags) really match the content to group together. By adding semantic tags to your document, you provide additional information about the document, which aids in communication.
- What would happen when the user click those learn more? In my opinion, clicking those "learn more" would likely trigger navigation not do an action so button elements would not be right. So you should use the
<a>.
- In order to center the card on the middle of the page , you can use the flex or grid properties and
min-height: 100vhto the<body>. Add a little padding to the body that way it stops the component from hitting the edges of the browser. No need for absolute positioning. Then you can use flexbox properties to the container that wraps the three card and give it flex-direction : row for the desktop and column for the mobile.
- If you make each column into a flex column. Then set everything inside the cards to have some margin in one direction
marin-bottom: ;only the link wouldn't need it and usemargin-top:autoon thelearn morelink that will push it to the bottom of the cards. - Really important to keep css specificity as low/flat as possible. It’s not recommended to use the ids to target the DOM elements for styling purposes, better to use classes so that it could be more manageable and reusable.IDs have a much higher specificity than classes) IDs have many uses in a web page aside from being a CSS selector.
- Add
border-radiusandoverflow hiddento the main container that wraps the three cards so you don't have to setborder-radiusto individual corners.
line-height: 40pxUse a unitless line-height value to Avoid unexpected results. You can read more in mdn
Hopefully this feedback helps.
0 - Use the
Please log in to post a comment
Log in with GitHub
Join 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