@gmagnenat
Posted
Hi, congrats on completing the challenge.
I have checked your solution and your repo. There are a few fundamentals you should check out that are important to understand. I hope it will help you refactor your solution and help you with the other challenges.
Does the solution include semantic HTML?
- a <main> landmark is missing. You should have one. The main element represents the main content of the body of a document or application. -w3c
- alt attribute on image are not ment to display error message. They are ment to describe the image for user who cannot see it.
- you don't need to wrap the image in an extra div here. it's contained in the card and the card have a small padding.
- the title doesn't use a semantic heading element. use h1, h2, h3, etc...
Is it accessible, and what improvements could be made?
- Width in pixel are used. You should use a max-width in relative units so layout can scale correctly on smaller screen.
- don't set a fixed height, let it scale with the content.
- you don't need fixed size on the image. it is contained in the card and will scale correctly with the card. just set a max-width of 100% to the image.
- don't need to set width on the heading. it's within the card content. if you want to reduce the width use the
chunit to limit the number of characters. - Why font-size must NEVER be in pixels. Use relative units for everything related to fonts and sizes in general it will make your layout respect user font size settings and scale correctly. Using pixels will not respect user preferences.
- don't need to set width on the <p> element. it's within the card content.
Does the layout look good on a range of screen sizes?
- It looks fine.
- A small padding on the body will make it look better on mobile so the card isn't sticked to the edge of the screen.
Is the code well-structured, readable, and reusable?
- You should use a modern css reset in every of your projects. Check out Andy Bell or Josh Comeau, they are both well structured for modern application.
- You should use classes instead of styling html element directly. It will be more reusable for larger applications.
- You can use flexbox on the body to center the content. Prefer using classes instead of styling directly the html elements. It will be more reusable in other part of a larger application.
Does the solution differ considerably from the design?
- It looks good and respects the design intention.
I know it's a lot of comments but I hop it will help you extend your research and understanding on some basics. Let me know on discord if you need more links to resources to understand the things I listed here.
Happy coding !
Marked as helpful
@idatenkiddy
Posted
@gmagnenat thanks for the suggestions, I actually tried to do it fast as possible due to time constraints. I was aware I used div instead of semantic HTML because i thought it wouldn't required for this tiny project, In future i will use Tailwind CSS as it gives me better advantage over making responsive layouts faster. Thanks for your dedication and time i appreciate it.
