Order summary component

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted over 1 year ago

  Hi, I hope this feedback helps

  1. There should be some space aroubd the card at all times. This is hitting my screen edges. You could add a little margin to the component or a little padding to its wrapper
  2. This is one component, meaning it is only one chunk of meaningful content. It cannot have a header element (that is a banner landmark on a page that should sit outside of main and it's purpose if for content that repeats on every page). And it should not have a section inside. This whole card could be one section if you wanted but there is no semantic benefits to doing that

  3. not necessarily a problem but can you explain why you've chosen the interactive elements you have done for change, proceed and cancel? What do you expect to happen on click of each of these? Do you understand the key differences between anchors and buttons, and when each should be used?
  4. you don't need a paragraph inside a button (I'm not even sure that would be valid html tbh)
  5. CSS looks good to me. The only things if consider changing are the inline-margins in the plan box. Id recommend using gap there and flex-grow on the middle item. I'd also consider adding in flex wrap as a defensive style for if/when text sizes are enlarged and the content may not fit on one line'.
  6. consider setting the max width in rem instead of pixels on the component
  7. consider making the top image width 100% instead of setting explicit px width in a media query

  Marked as helpful

  0

  ---

  

  Joachim•840

  @Thewatcher13Posted over 1 year ago

  Hi Grace

  Thanks for your feedback.

  1. If I test the project on the devtools from chrome it doesn't touch the borders (I tried different mobile devices on devtools)? I've set display on the body to : display: flex; justify-content: center; align-items: center;

  It should line-out in the center from the viewport always, no?

  2)I take care of this.

  1)I've used the interactive elements because the figma file had this on the design-active.

  2)Yeah, that p element should definitily not be there!!

  3)I'm gonna try this solution too!

  4,5) I will applied this on the project and remember it for other projects too!

  Your feedback was really helpful!

  0

  

  T

  Grace•31,270

  @grace-snowPosted over 1 year ago

  @Thewatcher13 well done

  Do you understand the key differences between anchors and buttons, and when each should be used?

  I've noticed the main card image needs object-fit: cover; too. This will stop it getting distorted/stretched on some views

  Also, there are errors about asset loading in the console. It looks like the path is incorrect for the body background image and you are trying to load the mobile bg image on all screen sizes. The path should be url(../images/pattern-background-mobile.svg)

  0

  

  Joachim•840

  @Thewatcher13Posted over 1 year ago

  @grace-snow ,

  An anchor tag is used for items that lead you to new pages and a button for anyhting else? So I think the payment button should be an anchor tag and the change and cancel a button?

  I should take a look to that image, gonna do that in a moment.

  Good reminder about looking on the console! Gonna fix that path indeed.

  I'm currently working on the workit landing page, so I reapeating Grid for now and the stacking concepts.

  0
• 

  Faisal Ahmed•410

  @faisalahmed11Posted over 1 year ago

  @Thewatcher13, your submission is greatly appreciated, and you are right on the learning progress, that reduces the difficulty level. and finally, I would say <p> tag is the clear winner and an easy way to go in this situation.

  0

  ---

  

  Joachim•840

  @Thewatcher13Posted over 1 year ago

  thanks! :) @faisalahmed11

  0