@gmagnenat
Posted
Hi, congrats for completing your second challenge !
I'll list here a few points that you can review to improve this solution and your future challenges. As always, reach out on discord if you have any specific questions.
Does the solution include semantic HTML?
- wrapping the whole card in a <a> tag isn't recommended as it puts all the code inside this link. You should keep a clean html. Use pseudo element on the a tag to expend the hitbox. The link is the title of the card.
- <header> has no special use here and act similar to a div. If you want to use its landmark capability it has to be outside of <main> where it will take naturally a "banner" role. use <header> for repeated content on multiple pages such as navigation for example.
- You should not use <div> for text content. use <p> elements. Keep divs to wrap elements for styling.
- Your h1 should be the link of the card and have an hover / focus state.
- When an avatar is next to the name you don't necessary need to specify the alt as it's purely decorative. Adding a detailed alt with the name of the author will just be repetitive when using a screen reader.
Is it accessible, and what improvements could be made?
- fix the link issue and test your solution by using keyboard only. You can check your solution with a screen reader or voice over. Try it, it's really interesting.
- use a max-width in
remcurrently using pixels don't allow your card to scale if the default font-size is changed by the user.
Does the layout look good on a range of screen sizes?
- yes
Is the code well-structured, readable, and reusable?
- It looks ok.
- You should add the reset css at the top of your stylesheet, just use one file. Avoid the browser to do multiple requests for styles.
Does the solution differ considerably from the design?
- The solution respects the design intention.
I hope you find something useful in these comments.
Happy coding !
Marked as helpful
@defPhisy
Posted
@gmagnenat Thanks, it helps so much to get feedback like this. Learning a lot. Can´t wait to work on these topics after a short vacation.
@defPhisy
Posted
@gmagnenat my vacation took a bit longer 😁, but now i am back to proceed. Tried to solve everything you mentioned. Maybe you've some time for feedback again:
Semantic HTML
- ✅ Replaced <a> tag into h1, what is better putting h1 into <a> or vice versa?
- ✅ Removed header, thanks for clarifying its purpose.
- ✅ Replaced <div> with <p> on all text elements.
- ❌ h1 has focus and hover, but :focus seems to have side effects i do not want. I just want :focus to be shown with keyboard. But it acts like :active if i use text-decoration or outline. The only exception is outline:auto. And in this case only on chrome. Beside that :focus behaves different on firefox, safari, can i solve this behaviour?
- ✅ Changed to alt="", or should i remove alt completely?
Accessibility
- ❌ chrome and firefox work with keyboard, safari not
- ✅ changed max-width to rem. But here i am not sure if my solution is good. I used calc() to convert my pixel value into rem because i want to be sure that i am sticking to the desired width from the designer. The thing here is that i have to hardcode the root font size of 16px and i think this is bad if some one has another root font size. Could not find best practice to that.
Structured Code
- ✅ css reset copy into main css. does it make sense to import the css reset into the main css or is this just shifting the problem. Just asking because it seems cleaner to add the reset with one line in the main css.
