@gmagnenat
Posted
Hi, congrats on completing this challenge!
There are some part you can improve in this solution. I hope my comment will be helpful.
HTML
- not sure button is usefull for the category here. maybe a link if it's ment to go to an archive page or just a <p> tag.
- This is a preview card component that will most likely lead to another page with the full article. Where would you put the link in your markup?
CSS
- I think the @font-face rules are correct.
- There is a problem of accessibility as this layout is not scaling if the user has change the default font-size setting of their browser. You need to get used to relative units like
remfor everything related to font-sizes and sizes. - You used clamp(), this is good but you should convert these values and not use pixels Why font-size must NEVER be in pixels
- There is an hover color change on the title but, is it a link?
- The active state of the card is missing.
- If you try navigating this card with keyboard only you can only get to the learning button and then to the attribution links.
Let me know if you refactor and if you want a recheck.
Happy coding !
Marked as helpful
@acandael
Posted
hi @gmagnenat ,
Thanks for your elaborate feedback. I implemented most of your remarks, the only thing I'm not sure of is how to fix the keyboard navigation.
Greetings,
Anthony
@gmagnenat
Posted
hi @acandael, good refactor on most of the font sizes!
You still got some pixels value there. There is still an issue with your card width.
To understand the issue, read the article I linked and understand how to change the default font size of your browser. For example, switch your default font size in firefox or chrome to 30px instead of the default 16px and check how your layout behaves. You will notice the issue with the width as you still have a fixed value in pixels. Use rem instead with a max-width so the card can scale.
You added a link on the h1 so you can now navigate with keyboard. Sidenote: with keyboard navigation you will first stop on the "learning button" now you have to think, "does it make sense?" if you don't see visually the card it could be confusing.
Sorry for the confusion about active state I ment changes when there is an action with that card so a hover would be more appropriate here I think.
If you want to push it further and learn some more stuff. Try to really think in term of accessibility. This card is probably one in a list of many, linking to plenty of different articles. So it can be good to have the whole card acting as a link to the article instead of going into the content of each card to find the link. This is an interesting challenge if you want to push this one further and a good pattern to know. Just a small tip : "wrapping the whole card in an <a> tag is not an accessible solution"
Marked as helpful
