Responsive recipe card

Community feedback

• 

  induwara-thisarindu•190

  @induwara-thisarinduPosted 5 months ago

  Hi good job on the submission first of all I want to say I am no expert so I will help as I can. In your css I noticed that you have written this,

  h1,h2,h3,p,div,img,section,article,main,ol,li,ul {
      margin: 0;
      padding: 0;
  }
  

  I believe it will be better if you do it like this,

  *{
      margin: 0;
      padding: 0;
  }
  

  I believe It is much more easier to write and time saving.

  Also, I think adding a height to body is what is causing the problem try removing that line and see I think the image will be visible in every screen

  html,body {
      height: 100%;
      margin: 0;
      padding: 0;
      overflow: auto;
  }
  

  The problem is that you are adding an overflow to both html and body maybe just add it to body like this

  body {
      overflow: auto; /* Ensure content is scrollable if necessary */
  }
  

  That's all I have to say Hope this helps 😊🎉

  0

  ---

  

  P

  Aaron Smith•280

  @medic-codePosted 5 months ago

  @induwara-thisarindu

  Thanks for the feedback!

  For the first change I think i'm being a little lazy here as it feels a little shotgun to use normalize css for these small projects, but yes indeed i may just use * { } instead from here on. I'd have a proper CSS reset in a live project.

  Thanks for the tip about height:100% I think I was using this to center the card with flexbox, as you need to define the parent container as having a height of 100% or 100vh. Your suggestion does get me closer to the overflowing problem it but will need to think of a different way to center.

  I suspect i wont need the overflow setting on html,body if i remove height:100%.

  Gets me a little closer, thanks!

  0

  

  induwara-thisarindu•190

  @induwara-thisarinduPosted 5 months ago

  @medic-code do you just want to center the card?

  0
• 

  Jo_WithVision•90

  @Jo-with-visionPosted 5 months ago

  Hi, well done on the submission - there a few suggestions from quickly looking at your code:

  CSS

  • You don't need flex for any of the card contents.

  It should all flow in a stacked structure naturally because it's mainly block-level elements, the exception being images, which aren't block-level. But with a CSS reset that includes setting images to display: block; and max-width: 100%, they also will not be a problem.

  I suggest duplicating your entire project as a backup and then starting over with a new version by taking all the flex off of your rulesets (you can leave flex on the main container if you want to justify it centre). Remove all padding and margins for the content and just see what the content inside the recipe card is naturally doing by itself. Then you can decide, section by section if/where padding/margin etc is needed.

  Sometimes we can overcomplicate things by ADDING properties we don't need (like too much flex).

  • You can take a look at mine to see how I resolved the marker indent. I just used padding. But it's important to note that padding won't work on a li item if you have other rules overriding it (remember CSS specificity) or flex complicating things.
  • Don't use pixels, only rem/em for your max-width sizes.
  • Remove the body height of 100% (you should only need to use, if anything, min-height: 100vh) on the body/a container when using flex/grid to centre an element in the viewport. You don't need to set a width of 100% for your recipe card. Just a max-width in rem/em (remember the main .recipe-card should be an <article> not a <section>.
  • If you choose to simplify your project vs 2 and remove all the flex, you will need to revise your media query. The media query should be very minimal adjustments for this challenge (some people don't use one at all).

  html

  • Your html structure could do with revising. You have the entire card in a <section> and then an <article> inside that. Really, you should have a <main> element, and then the card would be best wrapped in an <article>. You can then use <section>'s to logically section the inner content of the card, instead of using <div> elements, which have no semantic meaning.
  • The <span>'s should be <strong> elements. They have semantic meaning to make the word more important (it's an important inline "heading" to the instruction) and come with a bold style by default so no further styling would be needed on them.
  • Try to give your class names more useful meaning; you have names like "text" but really a more helpful class name would be .instructions, .instructions-text etc. It helps making reading code easier for not only you but other people.
  • Don't forget in your <table> that calories, carbs etc are the headings and can be placed in a <th> cell to distinguish them from normal <td> data cells. You can then add the scope attribute to each of the <th> to tell screen readers if the heading is for the row or the column. (In this challenge they are for the row). It would look like this:

  <th scope="row">Calories</th>
  

  repo url

  • Your repo URL for this challenge is pointing to a repo containing all your challenges as sub-folders. It's ok that you decided to make one large repo - but it may get rather complicated the more challenges you do and people will have to hunt for the correct challenge to review in the list. Point the challenge URL to https://github.com/medic-code/frontendmentor/tree/main/recipe

  Good luck!

  0

  ---

  

  P

  Aaron Smith•280

  @medic-codePosted 5 months ago

  Thank you so much for the detailed feedback, it really means a lot that you spent this time doing it @Jo-with-vision in other lines of work i have done the same for others so it really is appreciated.

  With regard to CSS think you suggest a good approach to see what the normal flow of the card does prior to flexbox usage, i do need to remember this generally.

  I will take on board your HTML feedback, all seems sensible.

  With regard to the repo url - makes sense to point the challenge url to the specific folder - not sure why i hadn't done that previously!

  1
• 

  ahmed safwat•230

  @ahhmedsafwatPosted 5 months ago

  hello there, the photo isn't visible in mid to x-large screens

  0

  ---

  

  P

  Aaron Smith•280

  @medic-codePosted 5 months ago

  Thanks for your feedback, indeed this was part of the feedback I gave myself - The figma design does not explicitly deal with mid to x large screens so whilst it works for 1440 x 1955px - I struggled with getting it to work with mid sized screens - any feedback would be most welcome@ahhmedsafwat

  0