Grace

Hello

Here's what I changed in browser (in a rush about to start work so sorry feedback is messy!)

.overlay img {
  note: does not need alt text;
  note: this would be much better as a pseudo element. It does not need to be in html;
}

.photo-container {
  /* width: 90%; */
  /* margin: 5% auto; */
  note: you should be using padding on the whole card, not margin here. I recommend to use rem, not % as you will end up getting unpredicable results.;
}

.footer p {
  /* margin-top: 0px; */
  margin: 0;
  note: Use a css reset at the start to remove browser specific styles like margins on elements. Take control of your styles.;
}

.card {
  /* width: 350px; */
  /* height: 600px; */
  /* align-items: center; */
  max-width: 18rem;
  padding: 1.5rem;
  margin-bottom: 1.5rem;
}

h1 {
  /* font-size: 22px; */
  font-size: 1.375rem;
  note: Its invalid html to have an anchor tag wrapping a heading element. Do it the other way around, anchor inside heading.;
  margin-bottom: 0.5rem;
}

body {
  /* font-size: 18px; */
  font-size: 1.125rem;
  line-height: 1.4;
  padding: 1rem;
}

.content-text {
  /* width: 300px; */
  /* height: 85px; */
  /* margin-bottom: 30px; */
  margin: 1.5rem 0;
}

.counter {
  /* width: 300px; */
}

.footer {
  /* margin-top: 15px; */
  /* width: 300px; */
  margin-top: 1rem;
}

p {
  margin: 0 0 1rem 0;
}

Overall

• use a css reset at the start
• Not everything needs to be a div
• Note what properties like align-items do on a flex column
• Try to use padding to stop content hitting edges (usually padding is in all directions, or one axis)
• Try to use margin more consistently - usually it’s only needed in one direction, top or bottom, or sometimes on one axis eg vertical only)
• Use rem! Anywhere you would otherwise use px, but extremely important not to use px for font sizes

I hope that helps, good luck

Following on from the feedback you requested 😊

• it’s easier to review the html and styles separately, so try to put css in a separate file rather than having all in the document head
• this only needs 2 heading levels. The paragraph that starts “30 day…” should not be a heading in my opinion. Both other headings “belong” to the original heading so should both be h2s. Remember heading levels are not about size they are about document structure, giving meaning to the content
• On your questions about sections, it would indeed be invalid to have sections within sections. Sections actually don’t get announced by most assistive tech unless they are aria-labelledby a heading, so using them doesn’t add much value semantically at the moment. It’s fine how you have it.
• the rest of the html looks good but I think it’s a real shame you’ve approached this challenge with flexbox when it is perfectly suited to css grid. That would get rid of the extra div (components-all)
• always include a css reset at the start of your css to remove browser styles. That will make spacings like margins on headings and paragraphs much more consistent and controlled.
• Never ever write font sizes in px. Use rem most of the time (occasionally em may be appropriate). This is important to ensure font always scales correctly when people have different zoom or text size settings. It’s actually good practice to use rem anywhere you might otherwise use px, so that everything scales together.
• this is causing big problems: width: 50vw; (and when 80vw). You don’t need any width on that other than 100% which it should already be by default. It just needs a max width in px (or preferably rem).
• on the same main element there is no need for position relative
• on the same main element there so no need for the margins (maybe just some margin bottom to push away the attribution)
• the body is missing it’s background color. Also it should have min-height: 100vh; and either flex or grid properties to vertically center content on the page. The body also needs a little padding, like 1.5rem to stop content hitting the screen edges (now we’ve removed the width from the component and used max-width)
• grid doesn’t make sense as a class name for what you’re doing at the moment - should that class name be used on the main element perhaps? That would make more sense if it’s being used to create an actual grid.
• padding should be the same on every box in this design so all boxes can have the same class
• don’t use key words for font sizes. Again, that’s not controlled and consistent. Use rem.
• it is better next time to work mobile first and use a min-width media query instead to override styles for desktop. That will almost always leave you with shorter and more performant code.
• all the media query needs to do in this is change the grid template if you were using css grid, or change the flex, direction if using flex. In this challenge you can actually do it with no media queries at all I think but that’s unnecessarily hard and would potentially end up with even longer code. So don’t worry about using them (in your slack question) - just use when you need to it’s fine

There you go - feedback as requested! Good luck

This looks like a nice neat solution when reviewing the code. Only a few small issues I spot with html

• The buttons should be anchor tags I think. They would almost certainly trigger navigation to another page (unless you think they would do something different?)
• Why h3s? At least wrap in a section with a visually hidden (sr-only) h2 to demonstrate that you think this component would sit lower down a real web page. I know this isn't a full webpage, so might not have a h1, but it is very important that headings go in order, so try to demonstrate that with your content (if in doubt, always choose a heading higher in the hierarchy like h2s for these headings, with an sr-only h1 above to name the component example)
• Make sure you undate the attribution link to point somewhere - e.g. your Frontend Mentor profile or github profile page

Good job on the code though, it all looks very neat and easy to read

Hello

It looks like you’ve got a typo in an essential meta tag which means I can’t see this on mobile (it’s the same issue that’s flagged in the report) card="width=device-width

Hello

This looks quite good except is broken on mobile landscape because you’ve used height instead of min-height (with the calc off viewport height). That’s easy to fix 😊

Other issues

• html semantics is lacking on this. As well as the missing main landmark flagged in the report, you should never have text in a span/div alone. A heading tag would be most appropriate for victors name, for example
• with alt text you never need to say “image of”. It’s already on an image element so users know it’s an image. Just victors name will be fine in that case.

I hope that helps