PhoenixDev22• 16,990
@PhoenixDev22
Posted
Hi VaporDusk,
Congratulation on finishing this challenge. Great job on this one! I have few suggestions regarding your solution:
HTML
- Page should contain
<h1>. The<h1>is most commonly used to mark up a web page title. This challenge is supposed to be one component of a web page. To tackle the accessibility issue in the report , you may use an<h1>visually hidden withclass=”sr-only”.You can find it here.
- Don't capitalize in html, let css text transform take care of that. Remember screen readers won't be able to Read capitalized text as they will often read them letter by letter thinking they are acronyms.
- In this challenge, the images are much likely to be decorative. For any decorative images, each img tag should have
aria-hidden="true"attribute to make all web assistive technologies such as screen reader ignore those images .
CSS
- In order to center the card on the middle of the page , you can use the flexbox properties and
min-height: 100vhfor the<body>add a little padding to the body that way it stops the card from hitting the edges of the browser.
- 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.
line-height: 48pxUse a unitless line-height value to Avoid unexpected results. You can read more in mdn
- Add
border-radiusandoverflow hiddento the main container that wraps the three cards so you don't have to setborder-radiusto individual corners.
- It's not recommended to set fixed height to component, you almost never want to set it. let the content of the component define the height.
- An explicit width is not a good way to have responsive layout . Consider using
max-widthto the component that wraps the three cards inrem.
- Remember a modern css reset on every project that make all browsers display elements the same.
- Don’t Repeat Your CSS(DRY) is a good general principle to follow and eliminating duplication of css code should naturally be part of coding journey.
- Consider using rem for font size , it' not recommended to use px for font size as absolute units don’t scale for example 15px will always be 15px on the same device. Using pixels is a particularly bad practice for font sizing because it can create some accessibility problems for users with vision impairments.
Hopefully this feedback helps.
Marked as helpful
1
VaporDusk• 230
@VaporDusk
Posted
@PhoenixDev22 Hi, I've been trying to implement everything in your feedback but while working through it I've met up with some difficulties:
- couldn't implement flexbox properties to get the design view: there is gaps between cards or they stick to one of the sides;
- couldn't walk away from setting border-radius to individual corners. Setting border-radius to the container didn't work. Rest of the feedback work well. Thank you for your feedback.
0
