Design comparison
Solution retrospective
I tackled this challenge in a few hours... feel comfortable using semantic tags.
What challenges did you encounter, and how did you overcome them?Had a few difficulties with the image ... centering! but definetly I enjoy it!
What specific areas of your project would you like help with?Any suggestions or differents points of view are always welcome
Community feedback
- @grace-snowPosted 8 months ago
I'm afraid the html needs a total rewrite on this. It's really important to get HTML right as it's the foundation for everything else.
- All content should be contained within landmarks. This is a card (div or section) that must sit within a
main
landmark. Every page must have a main at a minimum. - Inside the card must be no header or footer or articles (or any other landmark). Look up what these landmarks are actually for, because it is definitely not this. This would cause a lot of confusion for screen reader users who rely on landmarks to understand and navigate page contents.
- There should be no IDs in this.
- The image must have an alt attribute. I consider this image to be meaningful content so would expect a description in that alt attribute. But if you think the image is decorative you can leave the alt blank. There is a very good post about how to write good alt text in the resources channel on discord.
- The persons name is a heading (not a header).
- All text must be in meaningful elements, never in divs or spans alone. The location and description are paragraphs.
- The list of links must be just that — an unordered list with anchors (links) inside each list item. You must use interactive elements for interactive behaviour. Articles are not clickable.
I wrote a post about translating designs into HTML which may be worth you reading. The basic principle is to always imagine the content without any styling at all - that should inform your html element choices: https://fedmentor.dev/posts/html-plan-product-preview/
Marked as helpful0@grace-snowPosted 8 months agoThe styling is also a bit broken because content is able to spill out of the card. This is really common when people first start styling but some base principles can help here too:
- Always use a modern css reset at the start of the styles in every project. Andy Bell or Josh Comeau both have good modern examples you can look up and use in your projects. (Choose one not both)
- Set background color on the body. It's rare if ever you need to style on :root or html and it's best avoided.
- Never limit the height of elements that contain text, including the body. This is what's causing the big overflow problem on this solution. Change height 100vh to min-height so the body height is allowed to grow beyond the viewport height when it needs to.
- Never style on IDs! That is not what they are for, leads to highly specific css, reduces reusability and can create a maintenance nightmare. Use single class selectors as much as possible for styling. If you want to understand what the ID attribute is for, I've written a post about it in detail on https://FEDmentor.dev.
- The max width on the component must be in rem. This allows the layout to work for all users, even those of us who have a different text size in our browsers.
- The component MUST not have a height at all! (See point 3 above). There is no need for a min height either in this case. Let the browser do it's job and decide what height is needed based on the content.
- Please have one css property per line. This is really hard to read at the moment. Indent/format code consistently. Your code editor can even do this formatting automatically for you with prettier.
- I recommend you avoid em units for now. You should only use em very intentionally and rarely - You only use it when you want a property to scale with the elements font size. You never use em for font size itself. That could lead to awful bugs in bigger projects because em compounds, meaning you could lose all control over font size as it changes in different contexts. Use rem for font size (and probably for most instances where you're tempted to use em for now).
- Remove anything to do with height and width on the articles and footer. All the links will need when you add them is padding and width 100% to make them full width. Definitely don't use min-width. That is very rarely needed.
- Font size must never ever be in px. This is extremely important.
Marked as helpful0@MarFandino84Posted 8 months ago@grace-snow Thank you so much for taking the time on this review... I will go through every point you made here... can´t emphasize how much this helps!! thanks !!
0@MarFandino84Posted 8 months agoAbsolutly loving your post about HTML!! thanks so much again for taking the time to reply a consistent and valuable view!!! @grace-snow
0 - All content should be contained within landmarks. This is a card (div or section) that must sit within a
- @Rhyz26Posted 8 months ago
Well done
0
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord