ReactJS -- order-summary

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. Layout looks fine in general and that starting animation looks great.

  Some suggestions would be:

  • Always have a main element that will wrap the whole content. main element helps users to navigate properly on your webpage.
  • You are missing the wavy-image on the background :>
  • The vector img should have been using alt="" on it, since the image is just a decoration. If an image only acts as decor, use alt="" on it, if the image adds content, then use meaningful alt value.
  • Avoid using height: 100vh on a large container, if you inspect you layout in dev tools at the bottom, hover on the body tag, you will see that it doesn't captures the whole layout, because the height of the #root is limited to the remaining viewport's height. Instead, use min-height: 100vh on it, this takes full viewport and will expand if it needs to.
  • I see that you used position: absolute on the card itself, I don't really think you should use this, since you could have just use display: flex on the parent, just using position: absolute makes hard for element to be captured.
  • The music icon svg should have aria-hidden="true" since it is just decoration.
  • annual plan should be a heading tag, because it gives enough content overview about its section.
  • For this one, I think using a tag instead of button for the 3 interactive elements are better, since you are not using a form element in here where button are used.

  There are things in here, html usage, css, that lacks attention, are you sure you are ready for react? If you use react, make sure that you have fundamentals first.

  But still, great work on this one.

  Marked as helpful

  0