Skip to content
OverviewSolutions
16
Comments
3
dinahbrito
@dinahbrito

All comments

  • dinahbrito•120
    @dinahbrito

    3, ah sorry, didn't realize it was a button. And yes, that's what I meant by flex box. Maybe give it another try? It makes things a lot easier when you figure it out

    Marked as helpful
  • dinahbrito•120
    @dinahbrito

    Proceed to payment is a button, so that’s good! I think you could’ve simplified the Annual plan block, by adding padding to all sides of the Annual Plan div which would force the elements inside to center within that div. That way you don’t have to add margins or padding to the elements inside. It’s kind of like creating a really thick invisible border around the elements inside the div.

    Feedback:

    • The background image is an svg file that is partially transparent. So it just needs a different background color added to the body of the page to match the design.

    • The background color is also missing from the main section which is the container for the card.

    • There should be an H1 tag, probably around Order Summary as it’s the title of the card

    • If you change the attribute div to footer, it’ll get rid of one of the accessibility issues. Footer is basically a semantic name for a div and it makes sense to use since it’s at the bottom of your page.

    • It’s awesome you’re using custom properties, but maybe simplify it a bit? Anything that you don’t actually use shouldn’t be there.

    • Same with the heading tags in CSS, if you’re not using it in your HTML, its not necessary to style it.

    I’m not really sure what problems you’re having with Flex Box, but so far it seems to be working? The only thing that might be a problem is that you might be repeating yourself unnecessarily. Like it might be easier to have a container div holding all the content under the image. Add padding so that they’re lined up on the sides. Then you wouldn’t have to center elements individually. I hope that helps and good luck!

    Marked as helpful
  • dinahbrito•120
    @dinahbrito

    This looks great for your first time! The design is pretty close.

    Some feedback:

    • The background image isn’t visible for mobile or desktop. Since your project is already inside of the folder “order-summary-component-main” you don’t need to include that in the url path. Here are some tips that might help: https://css-tricks.com/quick-reminder-about-file-paths/

    • HTML headings have to be in number order. So after an H1 tag, there should be an H2 tag instead of an H4. Then you can style the H2 to be whatever size you want.

    • The button tag should be used only for buttons, so the "change" link and the "cancel order" link should be made with an anchor tag instead. It’ll be less code as you don’t have to remove the button styling.

    • In the future look into “flex box” and “grid”, it will really help with making the mobile version of the design responsive.

    Great job!

    Marked as helpful