Skip to content
Submitted almost 3 years ago

Order-Summary

Jason Haygoodā€¢40
@jkhaygood
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

Looking for feedback, maybe a way to use less css.

Community feedback

  • Vanza Setiaā€¢27,795
    @vanzasetiaPosted almost 3 years ago

    šŸ‘‹ Hi Jason!

    šŸŽ‰ Congratulations on finishing this challenge! I have some feedback on this solution:

    • Accessibility
      • All the page content should live inside landmark elements (header, nav, main, and footer). By using them correctly, you can make users of assistive technology navigate the website easily. In this case, you can wrap all of it with main tag,except the attribution. The attribution should be lived inside the footer.
    <body>
  <main>
    page content goes here...
  </main>
  <footer class="attribution">
      attribution links goes here...
  </footer>
</body>
    • For any decorative images, each img tag should have empty alt="" and aria-hidden="true" attributes to make all web assistive technologies such as screen reader ignore those images. In this case, all images are decorative only.
    • Also, when you write alternative text, it should not be hyphenated. The alternative text should be human-readable. Some resources to learn about the alternative text.
    • I would recommend always having a value on any href attributes. You can use / as the value (you only fill one href).
    • Why are you having a button inside a link? Anchor tags are for navigation - moving to different pages or content on the same screen, while the button elements are for actions like opening a modal, submitting a form, toggling element, etc. Choose one of them.
    <a href="/" class="link"><button>Proceed to Payment</button></a>
    • 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.
    • Only use div and span for styling purposes.
    • Always use meaningful element (p) to wrap the text content.
    • Use rem or sometimes em unit instead of px. Using px (especially for font size) will not allow the users to control the size of the page based on their needs.
    • Styling
      • You need to use the generated link tag from Google Fonts to use the font family. Copy-paste the URL from the style-guide.md (or just click this) and choose the necessary font-weight.
    Note: Once it's done, you should have something like this:

<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=Red+Hat+Display&display=swap" rel="stylesheet">

Note: This is just an example.

    * To make the card perfectly in the middle of the page, I would recommend making the `body` element as a flexbox container.

    /**
 * 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 */
}
    • The disc should have a white background color.
    • About the background-size, I would recommend making it 100%. As far as I know, vw doesn't always work as I expect.
    • Best Practice (Recommended)
      • Write your code with consistent style (the indentation, in this case).
      • Two things that you can do to write less CSS code:
        • Write the styling using mobile-first approach (write the initial styling for mobile layout). It often makes me write less code.
        • Use CSS shorthand properties whenever possible.
      • Prefer unitless numbers for line-height values to avoid unexpected results. The MDN documentation explains it well šŸ‘.

    That's it! Hopefully, this is helpful!

    Marked as helpful
    1
    Jason Haygoodā€¢40
    @jkhaygoodPosted almost 3 years ago

    @vanzasetia First of all I really appreciate your feedback! The reason I had a button inside an anchor link, I was thinking proceed to payment would need to be a link to take the user to a different page but a button so the background around the link would be clickable as well.

    0
    Vanza Setiaā€¢27,795
    @vanzasetiaPosted almost 3 years ago

    @jkhaygood You don't need to do that. By default, the a anchor tag is already clickable.

    You can make the link look like a button. You can try this.

    HTML

    <a href="/" class="button">Learn More</a>

    CSS

    .button {
  display: inline-block;
  padding: 1rem 2rem;
  background-color: #111;
  color: #fff;
  border-radius: 0.5em;
  text-decoration: none;
  font-size: 1.25rem;
}

    The text and the background should be clickable.

    Marked as helpful
    1
    Jason Haygoodā€¢40
    @jkhaygoodPosted almost 3 years ago

    @vanzasetia I'll keep that in mind next time, thank you!

    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