Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

Recipe Page

P
Marcus• 110

@marcus-hill

Preview site(opens in new window)View code(opens in new window)
Desktop design screenshot for the Recipe page coding challenge

This is a solution for...

Recipe page
  • HTML
  • CSS
1newbie
View challenge

Design comparison

SolutionDesign

Solution retrospective

What are you most proud of, and what would you do differently next time?

I am proud of my semantic html, involving researching the different elements and how they should be used, such as the section tags and also the article tag for the recipe.

I am also proud of my ability to make the design responsive, where it switches from the desktop/mobile view with the use of media queries.

I believe the end result is pretty good and matches the figma design.

What challenges did you encounter, and how did you overcome them?

I encountered challenges with the various padding and spacing within elements, such as between the bullet point on the lists and the list item, having to add classes in order to achieve the right spacing as per the figma file.

What specific areas of your project would you like help with?

I would like some feedback on both my semantic HTML and also the responsive element of my design. Should I be using pixels for media queries and have I done it in the right way? What other changes should I make to have this as responsive as possible?

Community feedback

P
Kamran Kiani• 860

@kaamiik

Posted

Hi good job. I have noticed some points I wanna mention:

  1. On the mobile view add a margin-top, left and right to have a space from the edge of the page.
  2. This page does not need any header and header should not be inside the main. We have header, main and footer and these are siblings.
  3. You can simply put your text inside li. There is no need to use span unless there is a styled text.
  4. On CSS try to use a better CSS reset. Andy Bell's good.
  5. Never ever use px for font-size. Only use rems. https://fedmentor.dev/posts/font-size-px/
  6. Also for width and heigh also use rems and for spacing you can use em too.
  7. You don't need and you should not use width and height to limit your card. It's not good for the page if your content change. On height you only need min-height: 100vh; most of the time and max-width:[...]rem
0
P
Marcus• 110

@marcus-hill

Posted

Hi @kaamiik, thanks for taking the time to review my work.

  1. The design files do not show any space from the edge of the page on a mobile view, so I deliberately left none and started designing it for mobile first, before later adding the margin for desktop and tablets.
  2. Thanks for the feedback on the header, I didn't realise this had to be separate to the main element so I have removed it and replaced it with a div instead.
  3. The reason I used span was because I was struggling to apply the margin-left to the li, and several websites suggested nesting a span inside the li in order to achieve this, as I was trying to match the figma design spacing. Would this have been possible without the span as it was not working for me?
  4. Thanks for suggesting a different reset.css, I've made the necessary changes to Andy Bell's.
  5. I've changed all px's to rem's for font sizes. Thanks for linking to the article, I can see how this will impact accessibility now.
  6. Noted
  7. I've checked over my code and I don't think I am using width or height to limit the card, I was already using min-height: 100vh and max-width: 42rem on the card inside the media query, so I will assume this has been done the correct way. I was using width on the banner and also a few other elements inside the card.

Thanks so much again for your feedback, it is much appreciated and helping me to improve!

0
P
Kamran Kiani• 860

@kaamiik

Posted

  1. Yes you are right but still on the size between 600px to 678px needs a margin inline to have some little space from the edges.

  2. Yes you are right.

  3. Yes. It's ok completely. I thought that your .banner class is a text wrapper. @marcus-hill

0
P
Marcus• 110

@marcus-hill

Posted

Hi @kaamiik, thanks for this feedback. I know what you mean now with the requirement for a margin between 600px and 678px so I have added one with a size of 2rem.

I appreciate your help!

1
Andres Lamar• 150

@AndresLamar

Posted

Hey, everything seems good in terms of HTML semantic and responsiveness of your solution

0

Please log in to post a comment

Log in with GitHub
Discord logo

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord