@vanzasetia
Posted
Hello, Adam! 👋
Congratulations on finishing this challenge! 🎉
I have some feedback on this solution:
- Accessibility
- Good job on leaving the
alt=""empty for all decorative images! 👍 - Add
:hovereffect for all interactive elements. It's much easier to identify any interactive elements if there are hover effects on them. - The attribution should be lived inside the
footer.
- Good job on leaving the
<body>
<main>
page content goes here...
</main>
<footer class="attribution">
attribution links goes here...
</footer>
</body>
- Don't use
footerfor the card content since it is not a full webpage. This is one chunk of content that all belong together and in a real website would sit with other content. Thefooterwould be the attribution. - Create a custom
:focus-visiblestyling to any interactive elements (button, links,input,textarea). This will make the users can navigate this website using keyboard (by usingTabkey) easily. - There should not be text in
spananddivalone; instead wrap the text with a meaningful element. - Styling
- To make the card perfectly in the middle of the page, you can make the
bodyelement as a flexbox container and remove themarginfrom the.cardelement.
- To make the card perfectly in the middle of the page, you can make the
/**
* 1. Make the card vertically center and
* allow the body element to grow if needed
*/
body {
display: flex;
align-items: center;
justify-content: center;
min-height: 100vh; /* 1 */
}
- Add
paddingto thebodyelement to prevent the card from touching the browser edges or simply the card if filling the entire screen, on mobile view. - Best Practice (Recommended)
- Remove the commented code. If another developer (it can be you in the future) wants to improve this solution, he/she might get confused about whether or not the commented code should be used or deleted.
- Always use classes to reference all the elements that you want to style. Using
idis going to make your stylesheet have high specificity (hard to maintain).
That's it! Hope you find this useful! 😁
Marked as helpful
@AdamElitzur
Posted
@vanzasetia I added the hover effects, thanks for letting me know about the footer, I took it out. Also, I looked it up, and it said that having text in a div or a span is ok, but I'll try to remove them. Also, when I removed the margin from .card, the attribution is too close. If I add a margin-bottom, it isn't centered anymore. Is there any way to add a margin that doesn't affect the location of the card? Also, when I add a padding to the body, at some point the body gets over 100vh, so a scroll bar appears. I don't think that's good, so I took it out. Any other way to make sure it doesn't touch the edge? Thanks for letting me know about the best practices, it's good to know. Thanks for taking the time to help me!
@vanzasetia
Posted
@AdamEli It's okay to have text inside a span or a div. What I'm trying to say is that the main wrapper of the text content shouldn't be div or span
Example:
This is not okay
<span class="text">
I may not accessible or pronounced correctly by a screen reader.
<span class="text--bold">Me too.</span>
</span>
This is okay
<p class="text">
I am sure that I will be pronounced correctly by a screen reader.
<span class="text--bold">Me too.</span>
</p>
About the padding, as long as it is a vertical scroll bar, that's absolutely fine. If the mobile users see the site in landscape view, they obviously need to scroll to see the rest of the card content.
@AdamElitzur
Posted
@vanzasetia Ok, thanks! But how can I add margin between the attribution and the card while keeping the card centered? When I add margin, the card moves up a little, making it off center.
@vanzasetia
Posted
@AdamEli That's okay if the card moves up if you add margin to the attribution.
The card and the attribution are the children of the body element so both of them will be adjusted to be center (not just the card element).
Also, you don't need to make your solution looks the exact same as the design. Focus on the code quality instead.
Marked as helpful
@AdamElitzur
Posted
@vanzasetia Ok, thanks! Also, my responsiveness isn't working, maybe because I set a specific padding/margin. I have max-width set to 100%, but on small screen sizes, it just isn't working. Is there something I have to change? Maybe I should have done mobile first design, is that a better practice to use for these?
I haven't fully dived into responsiveness yet, I'm planning on doing some non-responsive to get pretty good, then CSS Grid, then responsive. However, I still want this submission to be right. I though ems were responsive, but I guess not. Thanks so much, Adam
@vanzasetia
Posted
@AdamEli I would recommend trying what works for the card (setting a max-width: 100% is one of the ways) and yeah, I would recommend doing mobile-first approach to write the styling. It often leads to shorter and better performance code. As a result, mobile users will no longer be required to process all of the desktop styles.
@AdamElitzur
Posted
@vanzasetia When I try that though, it is a little better, but the sizing is a little off, and when the screen gets small, the Change link gets pushed out of the card. Also, the card is too small at certain sizes, maybe around 400 in Chrome dev tools. Let me know if you have any suggestions, thanks!
