Order Summary challenge solution

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted about 3 years ago

  Hi Jarek,

  I recommend you

  • use semantic elements for page structure as suggested above but also for any text. Annual plan can be a heading and the price a paragraph.
  • only change should be a button. The others should be anchor tags I think. Look up the difference between the two elements.
  • remove all the extra divs on this. There seen to be a lot of them. Remember you can do things like background images and use padding on the same element to make sure content doesn't overlap the images.
  • as a general rule, use css grid for layouts that need to go along 2 axis, and flexbox when the layout only goes along 1 axis. This seems a little complicated at the moment
  • the change font size is really small for.me viewing on mobile
  • never have any font sizes in px. Use rem mostly. I see you are using em quite a bit - that should only be don't very intentionally when you want the size to inherit from a parents font size. I'm not sure you are using em for that reason here(?). It's not wrong or anything, just something to be aware of.
  • always add focus visible styles to interactive elements. Make them obvious so keyboard users know where they are on the page.

  Overall a good looking solution though, well.done

  Marked as helpful

  3

  ---

  

  Jarek•75

  @airpolandPosted about 3 years ago

  Hello Grace,

  All your feedback is much appreciated. Thanks a lot for taking the time to look through my solution and put some high quality feedback. I restructured the solution so I think the HTML should be much more clear now.

  Have a great day! Jarek

  0
• 

  Jason Heys•175

  @heyitsganyPosted about 3 years ago

  Hey, nice job. You've got nice and close to the design and it looks good!

  There are a couple of things that I've noticed that you could work on to improve it.

  • Firstly, you give your body an ID? This doesn't seem necessary as we can already style the body using body.
  • You can apply the background "wave" image directly to the body in the CSS. This allows you to remove an unnecessary div. (Along with the z-index: -10).
  • Instead of using aria roles, why not just use the elements themselves? The whole thing can be wrapped in a <main> element. Your footer can just as easily sit in a <footer> element.
  • Don't forget to change the cursor when you hover the button and link elements. (Using "cursor: pointer").

  Marked as helpful

  1

  ---

  

  Jarek•75

  @airpolandPosted about 3 years ago

  @heyitsgany Wow - that's some great feedback. Thanks a lot for taking the time to browse through my solution ;-) I understand each advice and will definitely consider updating the solution.

  1
• 

  Koey•75

  @PiotrKukuc12Posted about 3 years ago

  Music icon has a red border, just remove it

  Marked as helpful

  0

  ---

  

  Jarek•75

  @airpolandPosted about 3 years ago

  @PiotrKukuc12 - thanks a bunch... that was beyond rooky mistake ;-)

  0