Design comparison
Solution retrospective
So much but in terms of this project i would choose my separate css-reset.css file that i can use for all my new projects.
What challenges did you encounter, and how did you overcome them?Had a problem with a mysterious margin-bottom on my avatar img. After some inspection i found that this was a problem to the selectors i used.
Line 64-67 on style.css:
.blog-card img { border-radius: 12px; margin-bottom: 1.5rem; }
Line 109-113:
.blog-card-author img { width: 35px; height: 35px; border-radius: 50%; }
The problem was that.blog-card-author
is inside .blog-card
. So it took margin-bottom
form there.
My solution to this was adding a new class for the avatar img, because i did not find a good solution to do it with selectors without loosing readability.
What specific areas of your project would you like help with?Everything what i can improve.
But for now i think you can help me with the avatar img, that does not load on the github page. locally everything works.
Due to the missing avatar image i found out that the alt text cannot be shown because the avatar is so small. Is that effecting screenreaders? I guess not but maybe there is a workaround.
Community feedback
- @gmagnenatPosted 4 months ago
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
rem
currently 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 helpful1@defPhisyPosted 4 months ago@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.
1@defPhisyPosted 3 months ago@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.
0 - @DougHungerfordPosted 4 months ago
Try adding a . before /assets/images/image-avatar.webp just like you did for the illustration!
Marked as helpful0@defPhisyPosted 4 months ago@DougHungerford thanks that worked, just took a bit to refresh
1 - @UMo0HUPosted 4 months ago
it is nice that you were able to find the solution for your problem, keep going bro.
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