Skip to content
Submitted almost 3 years ago

Order Summary Card (HTML and CSS)

Adam Elitzur•110
@AdamElitzur
Preview site (opens in new tab)View code (opens in new tab)
This is a solution for...Order summary component
  • HTML
  • CSS
1newbie

Design comparison

SolutionDesign

Solution retrospective

Hi guys, for some reason, when the screen gets too small, the text doesn't wrap, and it just gets cut off. Any suggestions? It might be something with the margin, I'm just not sure what the best way is to do this. Thanks!

Community feedback

  • Vanza Setia•27,775
    @vanzasetiaPosted almost 3 years ago

    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 :hover effect 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.
    <body>
  <main>
    page content goes here...
  </main>
  <footer class="attribution">
      attribution links goes here...
  </footer>
</body>
    • Don't use footer for 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. The footer would be the attribution.
    • Create a custom :focus-visible styling to any interactive elements (button, links, input, textarea). This will make the users can navigate this website using keyboard (by using Tab key) easily.
    • There should not be text in span and div alone; instead wrap the text with a meaningful element.
    • Styling
      • To make the card perfectly in the middle of the page, you can make the body element as a flexbox container and remove the margin from the .card element.
    /**
 * 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 padding to the body element 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 id is going to make your stylesheet have high specificity (hard to maintain).

    That's it! Hope you find this useful! 😁

    Marked as helpful
    1
    Adam Elitzur•110
    @AdamElitzurPosted almost 3 years ago

    @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!

    0
    Vanza Setia•27,775
    @vanzasetiaPosted almost 3 years ago

    @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.

    1
    Adam Elitzur•110
    @AdamElitzurPosted over 2 years ago

    @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.

    0
    Vanza Setia•27,775
    @vanzasetiaPosted over 2 years ago

    @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
    1
    Adam Elitzur•110
    @AdamElitzurPosted over 2 years ago

    @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

    0
    Vanza Setia•27,775
    @vanzasetiaPosted over 2 years ago

    @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.

    1
    Adam Elitzur•110
    @AdamElitzurPosted over 2 years ago

    @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!

    0
  • Heli0s•670
    @zeerobitPosted almost 3 years ago

    Good work completing this challenge.. The card size is too big for mobile view, add a max-width in your media query to make it smaller .

    1
    Adam Elitzur•110
    @AdamElitzurPosted almost 3 years ago

    @zeerobit Thanks! Having the max-width isn't working, is there anything else I can try?

    0

Please log in to post a comment

Log in with GitHub

Join 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