@ThembaMtshelwaneSubmitted 5 months ago
Jo_WithVision
@Jo-with-visionAll comments
- @Jo-with-visionPosted 5 months ago
Hi,
Great work on the challenge, here's some little bits of feedback for your consideration:
Switching the image: So I see you used JavaScript as the solution to changing the image - this could have been achieved without any JavaScript at all, by using the html
pictureelement. That's something to consider if you weren't already aware of that.Responsive units: I would advise for best-practices to avoid using pixel units and use em/rem units for all font-sizes, padding, margins.
custom properties for fonts and font-weight I see you have created a class to apply your fonts and weight, but this could be more easily achieved in your custom properties alongside the colours like this:
--font-family-secondary: 'Fraunces', serif; --font-weight-700: 700;...and similar for the primary font and its weights
The benefit being you could then utilise the separate font-weight custom property elsewhere and also use Fraunces elsewhere with a different font-weight, instead of having them 'tied up' in the same class. Just something to keep in mind.
use max-width: we always try to avoid setting widths when possible. If a challenge needs a limit to the width of a component, set it as
max-widthnot justwidth. It is also becoming very standard to use em/rem for widths (including widths in media queries) instead of pixels, or even percentage. This is all to increase responsiveness and aid accessibility.button states:
:activeand:focusare generally included alongside:hoverto cover all ground. You will notice when using the tab key on your keyboard to tab to the button on your live preview, the button colour does not change. If you were to includefocusthen the colour would change when tabbing. This helps to indicate the state change to a user using a keyboard.extra challenge: You have included the SVG icon as an
imgin your html. Research how you could use a CSS pseudo element to include the icon in the button instead for an optional challenge. The benefit of using pseudo elements for decorative icons is that it keeps decoration clutter out of the html document structure.(If you decide to leave it in the html: As the button includes text saying what the button does, you don't need to include an alt description for the icon. You can leave the alt description blank like this
alt="". The assistive technology will skip it and just read the button text to "add to cart".)Happy Coding!
0 - @medic-codeSubmitted 6 months agoWhat are you most proud of, and what would you do differently next time?
Getting the design mostly accurate. i think i would've probably done less on getting the inbetween sizes working correctly and sticked to the desktop and mobile view only.
What challenges did you encounter, and how did you overcome them?Whilst i was able to get the desktop and mobile versions to the design, inbetween sizes of height were incredibly difficult to figure out how to fix the overflowing top part of the card. It looked like the flexbox was actually causing this problem and was unable to find a solution to center the card without, despite thinking about floats or using auto margin.
Additionally getting the padding between the bullet point and the text in the list was really challenging, solutions made to seem this was relatively easy but nothing seemed to work, using ::marker, using ul or li padding/margin. Not quite sure why!
What specific areas of your project would you like help with?Better responsiveness on intermediate desktop sizes, the card overflows on my own laptop for example.
A better way to add padding on the bullet point lists.
@Jo-with-visionPosted 6 months agoHi, 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;andmax-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-cardshould 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-textetc. 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 thescopeattribute 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 - @Adhik-6Submitted 6 months agoWhat are you most proud of, and what would you do differently next time?
I'm a beginner and for me it took me 4-6 hours to complete this project.
What challenges did you encounter, and how did you overcome them?Styling the list-styles - I learned about "::marker" pseudo-class that styles the list-style of the ordered list.
What specific areas of your project would you like help with?Feedbacks are welcomed.
@Jo-with-visionPosted 6 months agoWell done on completing the challenge, the finished design looks great!
I am still learning too and these challenges always take me much longer than expected, so don't worry about that - hopefully we will speed up with practise :)
HTML feedback:
-
You could consider using the
<article>element to contain the entire recipe instead of a section, and use<section>for the individual sections of the recipe. Anything contained within an <article> should be self-contained and meaningful outside its context. Example: you have the 'prep time' inside an <article>, but ask yourself if that section was taken way from the page, would it make sense without the rest of the recipe? -
you may want to check with someone more experienced if it's necessary to place your
<h1>and image in a<figure>(I can see why you chose to, but I am not sure its needed, but I could be wrong). -
Good semantic use of the
<h2>element and paragraphs and lists. -
Consider more meaningful class names so your CSS is more understandable when others read your code (e.g. .ingredients-list rather than .ul-2)
-
I think the
<hr>is an understandable choice as it is a meaningful section break, but placing the recipe sections in <section> elements could work well for this challenge too. You can then create the border with a bottom border to the section in the CSS and style it. Just a thought. -
You can use
<th>for the table headers instead of<td>to distinguish them as headings. You can also look up using thescopeattribute on the <th> to associate the header with the row or column (in this challenge the heading cell is the header for the row). Here's how that would look:
<tr> <th scope="row">Calories</th> <td>277kcal</td> </tr>CSS feedback
- I would organise your CSS file either in the main folder alongside your index.html for a small challenge, or in a dedicagted styles/css folder. At the moment you have hidden it in the design folder with the other files from Frontend Mentor.
- Good use of custom properties - you can even add your font-weights and font-family as custom properties too (and whatever else you repeat a lot in your code)
- use rem or em units not pixels (I would also use rem or em for padding, not percent)
- You can use a modern CSS reset which will include displaying your images as block level elements and max-width 100% to make images easier to work with. I like this CSS reset by Josh Comeau: https://www.joshwcomeau.com/css/custom-css-reset/
- well done on discovering the ::marker pseudo element!
Hapy Coding! Jo
Marked as helpful0 -
- @BigO-DevSubmitted 7 months agoWhat are you most proud of, and what would you do differently next time?
Despite this project being a bit more challenging than the other projects, I'm incredibly proud of how closely I matched the design. Next time, I would do something differently: give more thought to the layout and how to achieve the correct spacing.
What challenges did you encounter, and how did you overcome them?The biggest challenge was styling the bullet points and adding spacing before the text. I achieved this with the `` property and setting it to
What specific areas of your project would you like help with?outside.This allows you to adjust thepaddingas needed.Any feedback on best practices and responsive design is greatly appreciated.
@Jo-with-visionPosted 6 months agoHi,
The finished design looks great with lots of whitespace for readability. You did a great job matching the design (especially if like me you didn't have the Figma design file!) There is extra length on the screenshot because of the extra spacing you have compared to the design, but good work!
HTML
You have used
<main>and<footer>which are great semantic elements. However, elsewhere in your HTML, I can see that you are relying heavily on using the<div>element, which isn't the best practice. Here's some tips:- Consider using
<article>for the recipe. The<article>element is for self-contained content that can stand-alone and also be shareable (a recipe page would be a perfect example of this!) - The difference sections of the recipe could be placed in
<section>tags. - If you use a CSS reset which includes setting images to
display: blockthere would be no need to wrap images inside a div. You can give the image a class name directly for any additional styling. - Consider changing your
<span>to<strong>as the beginning words are important parts of the instructions and<strong>indicates their importance. - Never use a div for a table! There is a
<table>element for the very purpose of creating tables in HTML, alongside<th>for table headers and<tr>for table rows (and much more!)
Please spend some time reading about semantic HTML and correct tags for your content. Remember you don't need to use a <div> when there are better semantic HTML elements available. A good resource is MDN (I will share a couple of links)
MDN Table Basics:
https://developer.mozilla.org/en-US/docs/Learn/HTML/Tables
MDN page structure and Semantic HTML:
CSS
There's lots of great stuff in your css:
- custom properties
- rem units
- A basic reset
- max-width in rem
- Tidy and well organised rulesets
About the list spacing: I just added left padding to my
liitems to create the space, but your approach is very interesting too and something new for me to learn :)Here are some thoughts (I am not a CSS expert so they are only casual observations):
- you shouldn't need to display the recipe as flex for this challenge because its children will naturally flow in a stacked structure anyway, because they are all block level elements (apart from the image, which could be made to be a block element in your reset).
- If you brush up on your HTML tables you can then style its selectors and classes using CSS instead of your current approach - it is actually much simpler when using the correct table HTML because it comes with default table structure!
- I'm not sure your media query needs to change so much! I have seen some comments saying not to use a media query at all for this challenge. I did actually choose to use one on my own project for basic changes.
Happy Coding! Jo
0 - Consider using
- @Jo-with-visionSubmitted 6 months agoWhat are you most proud of, and what would you do differently next time?
-
I didn't have the Figma file, so I am pleased with matching as close as I could to the design.
-
The mobile reference image was different to the Desktop design, so I managed to tweak the code to match the mobile view
-
I used semantic html reasonably well, and made the table more accessible with the scope attribute.
I noticed these differences between the desktop and mobile design:
- No rounded corners on the article or image on mobile view
- Image takes up full width of its container on mobile view
- No margin (background colour not visible) on mobile
I solved that with media queries, but I wonder if there was a simpler or more dynamic way with clamp or calc or something...
- My ruleset organisation continues to feel messy or overly complex for a simple challenge. I know with practise I will work out a system and start to code and organise more efficiently.
- Using BEM is still a challenge, I can't figure out what to do when there are lots of repetitive styles in different blocks. Utility classes, or multiple selectors for a ruleset?
- How to achieve the mobile design differences without a media query?
@Jo-with-visionPosted 6 months agop.s. I don't know where I went wrong to generate a blank space at the bottom of my screenshot - it doesn't display that on the live site!
0 -
- @newbscantdevSubmitted 7 months agoWhat are you most proud of, and what would you do differently next time?
I can visualize stuff better and think about what should i do and tool i need faster like i get more used with it
What challenges did you encounter, and how did you overcome them?I find it hard to make replica of an original one because i can't measure things but its still looks great!
What specific areas of your project would you like help with?honestly i didn't think any of this so if you have any suggestions please drop it below
@Jo-with-visionPosted 6 months agoHi,
Congrats on completing the challenge. I like how you have customised the design!
Here are some tips after looking at your code (it is not exhaustive, hopefully you will get more feedback from more experience coders):
- Consider keeping your styles in an external stylesheet and linking to your stylesheet using the
<link>element in the<head>section of your html. - Consider if the semantic tags you have used describe the content in the most meaningful way. For example, you could consider making your main container a
<main>element. You can definitely cut down on your<div>elements; a lot of them (if not all of them) aren't needed to style the project. - Reconsider the use of an
<h2>for your location text. It is not a heading, so a<p>element would do just fine. - It is usual to put
box-sizing: border-box;on the universal selector*, if you intended it to act as a CSS reset - Consider
min-height: 100vh, instead ofheight: 100vh - Try to use responsive units like rem/em instead of px.
🔥Happy Coding and great design concept!
p.s. With regards to measuring - I am in the same boat as I don't currently have a Pro subscription. Taking out a Pro subscription would mean access to the Figma design files that contain the exact measurements. You could also open the images you get with the free account into Figma and draw little boxes between the elements and see what the size of the boxes is to get an idea of the padding/margins etc.
Marked as helpful1 - Consider keeping your styles in an external stylesheet and linking to your stylesheet using the
- @teresaspencerSubmitted 6 months agoWhat are you most proud of, and what would you do differently next time?
I'm pleased that I managed to get my project looking very close to the original image example
What challenges did you encounter, and how did you overcome them?I had trouble getting everything to size correctly with the rounded edges. I used a lot of trial and error to get things right, and I'm happy with the results
What specific areas of your project would you like help with?I would like help explaining how to change the sizes and shapes properly without making strange things happen to my site. Another thing I would like feedback on is if my site is properly responsive, and if not what changes do I need to make?
@Jo-with-visionPosted 6 months agoHi,
Well done on completing the challenge!
In addition to the above - you could consider refactoring your code to:
-
include all your styles in an external stylesheet and connect it to your html file correctly using a
<link>element inside the<head>section of your index.html. You have a stylesheet called styles.css that is not currently connected to your HTML. -
Delete fontstyle.css from your repo as it does not contain valid CSS and the file is redundant. You can link to the fonts of your choice via a
<link>element in the head section of your index.html or via@importin your CSS. I can see you are already linking to the Google Font 'Outfit' in your index.html. -
Tidy up your repo further by removing files and folders you don't need such as the original README template that you are not using and the design folder. (optional step for organisation).
Happy Coding ✨
0 -
- @studiousvishnuSubmitted 6 months ago@Jo-with-visionPosted 6 months ago
Hi @studiousvishnu,
I've been asked to leave feedback for your project as part of the 'Introduction to Frontend Mentor' learning path, but it looks as though your project is still under development. Good luck and I look forward to seeing the finished project!
0 - @iwedibahSubmitted 6 months agoWhat are you most proud of, and what would you do differently next time?
i am proud of the speed i used to get the work done the neatness in my codes got better and my consistency
What challenges did you encounter, and how did you overcome them?major challenge has been in media query
What specific areas of your project would you like help with?majorly on media query
@Jo-with-visionPosted 6 months agoHi,
As your design is very different from the brief, I can only leave general code feedback. I will concentrate on a quick review of the HTML and leave a review of your CSS to more experienced coders.
Your HTML is nicely structured and formatted, with good use of some semantic elements such as headings and paragraphs. Here a few extra suggestions:
- Consider using more specific semantic tags where appropriate, such as <nav> for navigation, <main> for the main content, <section> for each section (e.g. WhatsApp, Snapchat, Instagram), or <article> for the individual cards would be appropriate as they are self-contained pieces of content. At the moment you are using a lot of <div> elements which do not have any semantic meaning for assisted technologies.
- Ensure that the colour contrast between the text and the background meets WCAG standards for better readability.
- Consider using more descriptive class names that indicate the function of the element.
Well done on completing the challenge!
0
