Petter Torst Saatvedt
@PetterTSaatvedtAll comments
- @afrad07P@PetterTSaatvedt
Hey Afrad, good work on your solution! I can see that you've put in some great effort on a rather challenging task, so kudos to you! :-)
When checking out your live website for the solution, I noticed some issues with the responsiveness of your solution when inspecting through DevTools. If not familiar with devtools, check it out here to check out how you can utilize it to test your solution on various screen sizes ;)
Here are some additional tips on how you could make your solution even better:
- Try to avoid using static units like PX for text, and rather opt for a scalable unit like REM to support users' needs when it comes to making text bigger or smaller (using px the text will always stay the same size, REM will scale nicely). 1REM is equal to the root font size, which by default will be 16px.
- I noticed you have set % values for width on you grid elements. As you have already set your grid layout in their parent element (grid container), it would be better to remove the width properties and simply focus on applying the correct grid-area names for your grid elements (which I see you already have done). Additionally, you should remove the set height value provided to the grid elements, as they will naturally fill their grid areas to maintain a consistent grid. You can notice the height issue by decreasing the screen width when viewing your solution! :-)
- The quotation mark for the first grid container seems to be positioned based on the entire page, rather than its parent element which it should stick to. Thus, setting a
right: 800px;
andtop: 50px;
will in this case base itself on the entire screen size, and will move accordingly when changing screen size. This results in the quotation mark moving to unwanted places. Try settingposition: relevant;
on the parent element, and then adjust the right and top values for your quotation mark :-) - Lastly, I noticed that you set your media query to include a max-width of 375px. Essentially, this means that your layout will change to the desktop grid for all screen sizes with a higher width value than 375px. Considering many mobile phones have a screen width around 400px (ish) your solution will not appear as the mobile version on these devices. Thus, I would recommend increasing the max-width of your media query to at least 480px which is the standard breakpoint for mobile to tablets. Considering this solution doesn't necessarily have a tablet layout, you could also just set the breakpoint to 768px which is the minimum width for smaller screens and laptops. I'd recommend testing out what breakpoints works best for your solution ;-)
Nonetheless, great work on your solution - and happy coding! :-D
- @FopefoluwaIkufisileP@PetterTSaatvedt
Great work on your solution, it works as intended on both desktop and mobile, with good responsiveness :-) Your HTML is semantic and well structured, which supports maintainability - well done! I also see some good approaches in your css, like use of variables, using rem to support scalability in text, and use of media queries for adjusting layout in relation to screen width.
If I were to pinpoint some areas in which your solution could become even better, it would be the following:
- Design: Spend a bit more time getting to know the provided design resources, and try to get as near the design as possible. As frontend developers, we should strive towards implementing pixel-perfect solutions from the designs which are given to us. For your solution, I would recommend setting a max-width of 22rem on the cards, to align with the design. Also, the shadow of the cards could use a bit of blur, to create a smoother shadow effect. Try upping the blur value (third value provided to the box-shadow property) to 1rem for example :-)
- DRY css: Although your css is very readable and well structured, I would recommend to follow a "don't repeat yourself" approach (DRY), which essentially means to minimize code repetition as much as possible. For instance, I see that you have made separate css classes for all four cards, which is understandable as they have some different positioning in the grid and different colors for the top border. However, values for properties like background-color, padding, position, and border-radius are equal for all four cards. These could be moved into a separate class, perhaps called "card", which could then be supplied to the cards in your HTML, alongside your more specific classes.
Overall, I think you have done a good job on this solution, and I wish you the best of luck on your coding adventure! :-)
Marked as helpful - P@sydalwedaieWhat are you most proud of, and what would you do differently next time?
- Use the `` element to create responsive images. It allowed me to dynamically load the correct image based on the viewport size.
- Use CSS Local fonts
- Use
element::before
selector and thecontent: url()
property to set the shopping cart icon. Bonus: It automatically sets the image to the appropriate size!
What specific areas of your project would you like help with?object-fit
property needs a set width and height on the element. I had a situation where the image container was shorter than the adjacent info container. I solved it by setting the height to 100% withobject-fit: cover
. The width was already set at 100%.When the viewport hits the breakpoint, sometimes I see a flicker as the browser tries to change the picture from mobile to desktop and vice-versa. I'm not sure what's the reason and how to fix it.
P@PetterTSaatvedtGreat work Sayed! The solution aligns well with the provided design, and functions well on all screen sizes :)
Your code is neatly structured which makes it easy to read and understand, while supporting maintainability of the code! I'm not seeing much that could be improved, to be honest, so well done!
As for the flicker you mentioned upon changing images, I assume this is simply due to the fact that the browser needs to load the other picture every time there is a change in layout. I would assume this could be fixed through caching of the images, so that they are already available when needed, however this would demand some JavaScript, which isn't necessary for this particular project :)
Keep up the good work!
Marked as helpful - @achecoP@PetterTSaatvedt
Great work on your solution Antony! It works well for both desktop and mobile screens, and you have done a great job in adapting the image for both designs.
Not much to complain about here! However, here are a few tips which could make your solution even better:
- H1 tags are typically reserved for the singular most important heading on the page, and should therefore only be used once. Consider changing all headings to H2 tags, except for the "Simple Omelette Recipe" heading which can stay as an H1.
- Try to avoid using px as a unit for font size. Since this is a fixed unit, it is not scalable for users who want to make the text bigger or smaller. I would recommend using a responsive unit like for example rem, where 1rem is equal to the root font size (which is 16px by default). This will allow for better scaling of the font size.
- As the solution includes a lot of information/text, it is important to provide sufficient spacing to let the content "breathe". I would suggest to revisit the design and follow the suggested spacing approach.
Nonetheless, your code is perfectly readable and easy to understand, and the solution looks great! Keep up the good work :-)
Marked as helpful - P@loki-pepeWhat are you most proud of, and what would you do differently next time?
I learned how to make initially inert elements tabbable.
P@PetterTSaatvedtWell done on your solution, Lovro! It is identical to the provided design, and works well across different screen sizes, so nothing to complain about here :)
I also went through your code and must say that it is very well structured and readable, and I particularly like your approach of utilizing css variables for consistency.
Keep up the good work!
- @PRINCE-OBOTWhat are you most proud of, and what would you do differently next time?
Using custom font and root pseudo class
What challenges did you encounter, and how did you overcome them?Understanding custom font was not easy for me, after series of explanation from ChatGPT I then got the concept.
What specific areas of your project would you like help with?Understanding Media Query more efficiently.
P@PetterTSaatvedtGreat work! The solution pretty much identical to the design, with the exception of what looks like a line height difference in the description. Nonetheless, the html is semantic, well-structured, readable, and covers important accessibility requirements!
Here's a few things I noticed while reviewing your code, which could be of use in future projects:
-
I noticed you were using a h1 tag in your component. As the h1 tag is usually reserved for the main heading on a web page (and assuming that the component would be part of a web page with multiple components) it would be better to use the h2 tag in this instance. Also, it is good practice to not skip header tag orders, so considering you are also using a h3 tag, updating the h1 to a h2 will now mean that you are not going directly from a h1 to a h3 :-)
-
I see that you are specifying font and text color for multiple of your css classes. To slim down the css file, and avoid repeatability as much as possible, you could add both the font-family and your "standard" font color inside the body selector, and remove all other declarations of those found elsewhere in the css file. Essentially, this means that all elements of the page will inherit the font and font color, considering they're all children (or children of children etc) from the body element. Now, in the instances of some text requiring different colors, you can keep the css as it is, as they will now override the inherited color :-)
Keep up the good work!
Marked as helpful -
- @FadiBarbahanP@PetterTSaatvedt
Nothing to complain about here, well done. The solution successfully encapsulates the necessary design considerations, and follows good approaches towards semantic HTML and accessibility. Cool usage of css variables!