Order Summary Component

Community feedback

• 

  Travolgi 🍕•31,400

  @denieldenPosted over 2 years ago

  Hey Arthur, congratulations on completing the challenge! You did a great job 😉

  Let me give you some little tips for optimizing your code:

  • add main tag and wrap the card for improve the Accessibility
  • for fix the top image in the background just put more specific background properties to the body:

  background: url("../img/pattern-background-desktop.svg") no-repeat top center;
  background-size: contain;
  background-color: #e0e8ff;
  

  • remove margin from body to remove the scroollbar of browser
  • use min-height: 100vh and not height
  • add transition on the element with hover effect
  • instead of using px use relative units of measurement like rem -> read here

  Hope this help! Happy coding 😁

  Marked as helpful

  2

  ---

  

  P

  Arthur•290

  @ArthurPogPosted over 2 years ago

  @denielden 'Sup Daniel! Thank you for the heads up. I did most of what you said and it looks great. I am going to have to research and understand rem a little bit more before using it in my code (that and CSS variables and native HTML tags). I only just got the hang of em so there's still some work to be done :)

  Thank you for helping me fix the background issues. It now works as I originally intended.

  1
• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  Hello, Arthur! 👋

  Regarding the problem that you have:

  • The music icon has a line height, maybe because the img element is an inline element. So, I recommend making the img element as a block element and (probably) setting line-height: 0 would solve the problem.
  • I highly recommend making the body element as a flexbox container to center the child element which should be the card element. Then, set min-height: 100vh; (**not height) to make the card vertically center and allow the body element to grow if needed. So, the point is to use min-height and flexbox instead. Also, add some padding as well to prevent the card from having full width on the small screen size.

  Some feedback from me.

  • All images on this site are decorative images. So, I recommend leaving the alt empty.
  • Don't use br elements for presentational purposes. Read what MDN documentation says about it. I would recommend making the span as block element to move the text to another line.
  • Always specify the type of the button. In this case, set the type of them as type="button". It's going to prevent the button from behaving unexpectedly.
  • I would highly suggest using HTML native element instead of role attribute. It's best to use the native HTML element whenever possible.

  That's it! Hope you find this useful!

  Marked as helpful

  2

  ---

  

  P

  Arthur•290

  @ArthurPogPosted over 2 years ago

  @vanzasetia Thank you, thank you and thank you, brother!

  I fixed the vertically centred issue that I had (as first recommended by @OGShawnLee). I deleted the alt descriptions, made the span blocks, specified the button type and deleted the div classified as main and just put it in a main native HTML tag.

  You rock!

  0

  

  Shawn Lee•560

  @OGShawnLeePosted over 2 years ago

  @ArthurPog I think you should have left them empty as he said instead of pulling out the big guns and completely remove them xD. An image must have an alt attribute, however you can leave them empty when the image is decorative.

  Marked as helpful

  2

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @ArthurPog You're welcome! 👍

  Yes, I agree with @OGShawnLee that img element needs an alt attribute. Just leave it empty. 😉

  Oh, I forgot to mention that put all the styling on the external stylesheet. Don't mix HTML and CSS (and also JavaScript). It's important because internal styling is hard to maintain (and not reuseable through out the project) because if you work with multi-page website and there's something wrong with one styling let's say on the body element then you have to remember to fix each styling on each HTML file.

  Marked as helpful

  2

  

  P

  Arthur•290

  @ArthurPogPosted over 2 years ago

  @OGShawnLee Thanks again! I returned the alt's back :)

  0

  

  P

  Arthur•290

  @ArthurPogPosted over 2 years ago

  @vanzasetia Yeah, I know CSS has to be separate. Just for this project and a few of the easier, one-page ones before I did it this way, because I was coding in codepen. Now that I moved to VS Code and using Live Previews, I will definitely save them in individual files. Muchos gracias :)

  0
• 

  P

  Arthur•290

  @ArthurPogPosted over 2 years ago

  Btw, does anyone have any clue why my generated screenshot is offset vertically by approximately 10 pixels? I have this with every project :)

  0

  ---

  

  P

  Arthur•290

  @ArthurPogPosted over 2 years ago

  @ArthurPog Just found out what it was if anyone is interested. Any padding or margin applied directly to the body causes this. I just moved the margin onto my main container and vois-lá!

  0