Hi there! I'm Angelo a this is my first work ever. Just one month has passed since I've start learning HTML/CSS by myself, so I'm super open to every good/bad feedback.
P.S. I'm from Italy, then I'm so so sorry for my bad English.
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
When it shows the design comparison, my app kind of looks like it fell apart but on my site everything looks fine, why is this? Otherwise, I think I was able to do this pretty well, but I didn't completely do the background and I definitely struggled on the container that has the music logo and the annual plan. Any tips for a more efficient way to build that part. Also, I'm not sure if the "Proceed to Payment" button was suppose to be a link but I just have it as a button. Also don't have a complete understanding of how my method of centering the app worked using flexbox. Anyways, any thing I should improve on and could have done better
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!
Hi there! I'm Angelo a this is my first work ever. Just one month has passed since I've start learning HTML/CSS by myself, so I'm super open to every good/bad feedback.
P.S. I'm from Italy, then I'm so so sorry for my bad English.
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!
