@gmagnenat
Posted
Hi and congrats on completing the challenge !
The result looks nice as a first sight, so congrats for that !
As you ask about accessibility i'll try to focus on this. It's a subject i'm very interested in. Accessibility is a lot about correct HTML.
Does the solution include semantic HTML?
- You are missing a <main> element
- I would just use an <img> element for the profile picture. not an empty div with a background image which doesn't tell anything to a screenreader for example.
- You should use <p> for your text content for name and title so it has a semantical meaning.
- You should have only one h1 per page then use lower level heading. You can probably use h2 for the item headings.
- You can use the blockquote element for the quote.
- The attribution items could be put in a <footer> element
Is it accessible, and what improvements could be made?
-
it scale correctly when the browser font-size is increased
-
Eveything should be included in a landmark. missing <main> as mentioned above.
Does the layout look good on a range of screen sizes?
- it looks good
Is the code well-structured, readable, and reusable?
HTML and class names
- About class names. Instead of "item-one" "item-two" you can use :nth-of-type() in your css and you just keep the item class.
- Same thing for "img-one" "img-two"... just use the <img> element with the correct image so you don't need these classes. Much simpler to handle and see what is happening.
- Instead of generic class names like "title" which could be a lot of different things if I don't know the code. Try to check some CSS methodology like BEM for example. the name classe could be something like author__name
CSS
- You need a modern CSS reset at the beginning of your stylesheet for browser consistency. Check out Josh Comeau or Andy Bell they both have a good modern CSS reset.
- The background pattern for the quote could be just added to an extra class. So you could move it to another quote for example.
- Using better classnames will help a lot to organise this and make it much more maintainable.
- If you work mobile first I don't think you need to set a min and max media query
Does the solution differ considerably from the design? The solution respects the design intention.
Things I liked to see
- good use of font sizes in rem
- min-height and max-width
- relative unit in media queries
I hope you find something useful to in these comments to improve your solution and your future challenges. Don't hesitate to ask on discord if some of the points here are unclear.
Happy Coding !
Marked as helpful
@Raptor0x1
Posted
Thanks for such a detailed and useful feedback.
Keeping all the things you have pointed out I'll refactor my code. It's helping me a lot. But I like to ask something about using <main> element. When this testimonials component will be used in a website there would already be a <main> element after the <header>. Inside the <main> element, <section> will come. So should I still use <main> in the current code enclosing the <section>? @gmagnenat
@gmagnenat
Posted
@Raptor0x1 sure, you want to add a <main> element in all your project to help indicate the browser where the main content is. Even in these projects its a good habit to take and doesn't change anything to the rest of the code.
