Order Summary Component using flexbox

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. Layout in general looks great, just needed padding on the bottom part of the layout so that it won't touch the flooring of the screen. Adaptive for mobile state but up to extent of only 20rem as you have made on the width on the layout.

  KC already gave feedback on this one, just going to add some suggestions as well:

  • It would be great to have a base styling of this:

  html {
    box-sizing: border-boxl
    font-size: 100%;
  }
  
  *,
  *::before,
  *::after {
    box-sizing: inherit
  }
  

  This way, handling an element specially its size will be easier because of the box-sizing

  • Zooming out, the background-image does not occupy the full size of the screen, you can use background-size: 100% on the body tag so that the background-image will fit on any size.
  • Using margin is not consistent enough to center a component, especially in y-axis, instead you can remove the margin on the .container and on the body tag add these stylings:

      background-size: 100%;
      align-items: center;
      display: flex;
      justify-content: center;
      min-height: 100vh;
  

  • Also, you have used multiple media breakpoints on this one, to be honest, you can just go with this challenge with no breakpoints or a single breakpoint, just to change the background-image. I think that is the only stylings that would require a breakpoint on this one. Have a though about that one:>
  • Always have a main element to wrap the main content of the site. For this use main tag on the .container selector.
  • To make the layout really adaptive for mobile state. When you are limiting a components size, instead of using width props, use max-width so that the component won't have a fixed size. For example, the .container should be using max-width: 28rem and that's it, it is not already adaptive.
  • The vector/illustration could use an extra aria-hidden="true" attribute on it so that it will be totally hidden for screen-reader users. In general, if an image is just decorative, use alt="' and aria-hidden="true".
  • Same goes for the music-icon, it is only a decorative image so better hide it using the method.
  • The change should be an interactive element like a button or an a tag since it is supposed to change the users pricing. For this one, if you think that this component is like a form, it would be better to use button since it will control the form, otherwise use a tag.
  • Lastly, just the padding on the bottom and top of the body tag to prevent the layout touching sides.

  Aside from those, really great job on this one.

  Marked as helpful

  1

  ---

  

  Joe•60

  @Joe-LindiePosted about 3 years ago

  Hello @pikamart,

  Thank you so much for all your feedback!!

  A few of the concepts were new to me like [box-sizing: border-box] and [box-sizing: inherit], but after some research, they seem a little clearer. I need to do a little more studying about box-sizing in general.

  I have updated most of the code on my Github. I'll try and remember all these tips and tricks for my next project. I think I will try and move from a newbie to a junior challenge for my next attempt.

  Thanks once again,

  Joe

  1
• 

  KC•90

  @kchettiar1Posted about 3 years ago

  Hey Joe-Lindie. You've been coding for two months - what a great start! Some quick feedback: the card (container) does not quite look centered to me?? I checked this on Google Chrome via Inspector on 1440 x 900 dimensions. The vertical alignment places it quite high on the page.

  The only other thing I could spot is on desktop view - the card container looks narrow compared to the design?! I can see you used media queries to adjust the positioning of the container. But I didn't see any width/sizing adjustments of the container.

  Anyway again well done especially only after two months! All the best in your web dev journey!

  P.S. If you haven't already discovered CSS Tricks is great resource in general. https://css-tricks.com/

  Marked as helpful

  0

  ---

  

  Joe•60

  @Joe-LindiePosted about 3 years ago

  Hello @kchettiar1,

  Thanks for the feedback, I appreciate it! :)

  You're right, the card container wasn't centered. I have centered it horizontally using margin: auto, and margin-top to center it vertically. (I'm not sure if there is a better way to center it)

  I have made the desktop view wider and added width to the media queries, too.

  Thanks for the link and the feedback!

  0