Hello, feedback as promised π
- You are misunderstanding how to use landmark elements in this. All content should be within a landmark, and every page should minimally have a
main
. This is one single component so everything inside it should be within main, that's not just for part of the card..grid_container
should be the main element - Do not set explicit widths on a component like this. Instead, give
.grid_container
a max-width only. - The image does not need wrapping in a div. Only add extra html when you need to
- The image has incorrect alt text. If you feel the image is decorative, alt can be blank. If you feel the image is valuable content for the page it must have a proper description of what it looks like
- As you're using grid for the grey box with all the pricing info in, you do not need to nest extra grids inside it! There's just no reason for all these divs. You're over-complicating it.
- The icon can have an explicit width and height and definitely does not need an alt description. It's decorative, so leave the alt blank
- I don't know why the price has had each part of it wrapped in a span. Again, avoid unnecessary html
- It doesn't look like you're used the colors from the design, and the sizing doesn't match. Try to get it all much closer. e.g. proceed is full width in the design...
- There's no reason to wrap proceed and cancel in divs
- As above, make sure you add hover and focus-visible styles to every interactive element. Focus-visible styles must include a noticeable outline on all sides, it is meant to be practical, not pretty, to help keyboard users know where they are on a page
Lastly, this is more of a question for you - if this was a real project what would you expect to happen on click of each of these? a) Change b) Proceed c) Cancel How you answer for each one determines what element each of those should be
Marked as helpful