Recipe page with HTML and Advanced CSS (combinators & more)

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted 28 days ago

  This challenge is primarily a HTML and accessibility focused challenge. But you're misunderstanding some parts in this.

  • if using the article element (unnecessary in this but find if you want to) it would need to wrap the img and all the text content (effectively the whole component in this design). The image is an important and relevant part of the content.
  • this doesn't have a header. The header element is a landmark when outside of main. That's when it has a banner role and is designed to hold content that repeats across multiple pages. But when inside main like this it is meaningless. There is no benefit at all to using it. This design doesn't include any content that would be suitable for a banner/header.
  • it's similar with all the sections too. They are unnecessary. You can have one container for all the text, an inner container for the preparation box and that's really all you need. By adding in sections (which are just like divs in this context) all it does is make the styling a little more complex.
  • headings should go in hierarchical order. So preparation should be a h2 not h3.
  • The one essential change that's needed in the html is to the nutritional table. You must use header cells for header cell content. As these are row headers in this case those header cells should also have scope="row" on them so it's clear they are row headers not column headers as is the default for header cells.

  I also recommend you read this about why resizing the html font size like you're doing isn't a great practice: https://fedmentor.dev/posts/rem-html-font-size-hack/

  It's also recommend

  • to include a full modern CSS reset at the start of the styles in every project you do. Look up Andy Bell’s modern CSS reset which would be very good especially for a solution like this.
  • This challenge should only need one media query if you’re adding more than one it’s a sign that you’re over complicating the CSS. The only changing media query should be where the padding is and the amount of padding on the component. By default on mobile, the padding would go on the single wrapping element around the Text. Then on larger screens, the padding would instead be on the whole component. I think the only other change for larger screens is that may need a bit of padding that’s about it

  Marked as helpful

  0

  ---

  

  P

  Facundo Andrés De Los Rios•200

  @FacundoDLRPosted 27 days ago

  @grace-snow Hello, thank you for the review and all the key points you left so that I can optimize my code and key points to better understand some important labels and measurements for web design and accessibility. That's why I took your time seriously and got to work.

  • change all REM measurements to the browser's default measurement.
  • clean HTML code (unnecessary tags) only leave a section for the "preparation" section since I did not find another solution to give the necessary style with a specific color background.
  • remove the unnecessary media query and now there is only one.
  • clean unnecessary CSS code and tidy up the code
  • I tried to use Andy Bell's CSS reset, but it didn't convince me since there were styles that I didn't like and I left a basic reset at the beginning of the CSS (margin, padding and border-box).
  • add the "scope=row" attribute to the header cell within the table in the "nutrition" section. I hope it turned out well, heh.
  • Modify the title tags so that everything is consistent.

  I hope you can look at my code again and leave me some comments. thank you =)

  0