Order summary responsive

Community feedback

• 

  Hania B.•1,280

  @techanthere

  Posted over 3 years ago

  Congratulations on completing another challenge! Few things I will like to highlight for help, you are almost overusing the heights and widths and then those too with the viewport units which is not ideal for every container. You should rather set min-height:100vh; in the body selector, and that's all you need. Other than that I don't understand why are you using fixed widths and heights everywhere, like using height:70vh; inside for middle_div is redundant. Your solution is breaking on widths higher than 480px. I think you don't need to set any media queries for this solution. Just set some max-width on .main, some padding in body selector and you are good to go.

  For html structure, span is not required inside the button instead you can directly style button text. Span is mostly used for applying different styles to a small text within the same paragraph, heading or button if there is some text different from the whole text inside the button. You should use anchor tag <a> or button for "change" option and same holds for others like cancel etc.

  Hope it helps!

  11

  Marked as helpful
• 

  

  Grace•32,130

  @grace-snow

  Posted over 3 years ago

  Hi

  I’m afraid I recommend starting this one again. Remove all the css you have, push fresh to the same repo for a clean break rather than trying to fix this as there is little recommended to keep (sorry to bring bad news)

  Here are tips for improvement

  • think through html semantics carefully. Images don’t have to be wrapped in divs, they can be set to display block directly (very common in css resets)
  • pattern can be background image on the body, no need for a div
  • use headings and paragraphs. Never have text in a span or div alone
  • as already said above, no need for spans inside buttons that only hold text
  • think about whether each interactive element should be a button or an anchor tag. Anchor tags would trigger navigation, buttons would trigger an action (like toggling content or submitting a form)
  • put the images inside the card in the html rather than using background img in div, I think you’ll find that easier
  • remove all heights and widths from everything. The only places you’ll need a min-height is 100vh on the wrapper element to vertically center your component on the page; maybe a min or max height on the image at the top of the card (probably not necessary if it’s set to width 100%); and an explicit height and width on the decorative music icons. Icons are probably the only place I’d regularly recommend giving exact heights and widths. Most of the time, you won’t need them.
  • use padding on the proceed to make it look tall enough. On that it could be in em or rem.
  • for paddings and margins don’t use viewport units, use rem. Viewport units should only be used sparingly for very specific purposes. This has caused most of the breakage in your solution I think

  I hope this is helpful

  02

  Marked as helpful
• 

  Can Surer•80

  @cansurer-at

  Posted over 3 years ago

  Hi Grace, Html fixed, I was using them randomly but now, I made some upgrades. I used the anchor tag by Hania B's advice. But I changed it with a button element now. I also corrected the cancel element. I mostly changed the margin and paddings. For horizontal things, I used margin. For vertical things, I used padding. All font-size's were changed with rem instead of px. Added an active page with the hover method.

  All advice's helped a lot and has a big improvement to me.

  Regards

  00