Skip to content
OverviewSolutions
23
Comments
290
Agata Liberska
@AgataLiberska

All comments

  • Luís Felipe Passamani Santos•10
    @passamanii
    What are you most proud of, and what would you do differently next time?

    I've made it.

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

    I really strugled to finish this project, even that this is a Newbie diffuculty one. I'm happy to say that despite the challenges and the over time spent, you can learn new things and get better every single day. Believe in yourself. And count on some other people to help you through the way. Peace.

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

    Html and mostly Css learning and developing.

    Simple Recipe Page

    1
    Agata Liberska•4,075
    @AgataLiberska

    Hi, I think visually your solution looks pretty good but I think things could be improved in your HTML and css. Firstly, your recipe card shouldn't really be using <main> as the wrapper, you should have an <article> tag in there (inside the <main>, not instead of it). You could also use <section> tags inside the article for the ingredients list, instructions etc.

    With your CSS, with your next challenge, I would start with looking over the design to see what styles are repeated throughout.

    For example, here we have a <h2> which is used 3 times and they visually look the same, but you're removing the default margins in your reset and apply different padding-bottom to each of them to make them look the same. Also we have two <hr> which have different styles - again to make them look the same. These are just examples but it overall seems like a chunk of your styles is just overriding the styles you set earlier and it's just unnecessary I think - have a look over to see if you can simplify things. Don't worry though, this is something that definitely comes with practice!

    Marked as helpful
  • Dorm-24•90
    @Dorm-24
    What specific areas of your project would you like help with?

    None, I just want people to see my solution and tell me what their opinion is.

    Solution

    1
    Agata Liberska•4,075
    @AgataLiberska

    Hi, your solution looks great! The only thing I noticed is that at a certain width the card stretches from edge to edge in the viewport, would be nice to have some space around it. It should also definitely be wrapped in an <article> tag rather than just using <main> as the container for it. I'd also consider some more semantic classes than .container1 and .container1 (even something very simple like .img-wrapper and .content-wrapper, because then just looking at the class I can guess what they will be).

  • Moundjid Machghour•440
    @PxMach
    What are you most proud of, and what would you do differently next time?

    I'm proud to have done this project. If I had to improve anything else, I'd work on : Dynamic formatting so that the display changes visually when a value is updated.

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

    Dynamic updating of values. Problem: Initially, tipAmount and totalAmount were not linked to the DOM, so their values were not displayed.

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

    If you have any suggestions, I'd love to hear from you.

    Frontend-Mentor-Tip-calculator-app

    1
    Agata Liberska•4,075
    @AgataLiberska

    Hi, your solution looks good visually but I don't think it full works as intended. If I pass the total bill amount in the first input, the tip amount gets split per person but the total doesn't, so that would be something good to fix.

    Some other improvements that you could do:

    • your inputs could use some hidden labels
    • also would be good to have a min value set on them so that I can't put a negative value in, you check for this in your js which is good, but there is no feedback to the user about this being incorrect input
    • I'm also thinking the buttons you have could maybe be radio inputs rather than buttons, and then they could be wrapped in a fieldset to have a semantic link between them in html

    I would say there is nothing really wrong with your js other than it may be a bit heard to read by a person, maybe in your next challenge before you start writing actual js, you could start some pseudocode, write down what needs to happen in each step so you can logically separate them. For example here, it would be nice to see functions called something like calculateTipPerPerson(rather than just 'tip') or calculateTotalPerPerson - when you work in a team it's important for everyone to be able to follow the logic you followed :) hope this helps!

    Marked as helpful
  • One nne•50
    @Onenne1
    What are you most proud of, and what would you do differently next time?

    The use of custom CSS properties

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

    Trying to vertically align the maker on the <ul> to always be at the middle of the text

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

    Trying to vertically align the maker on the <ul> to always be at the middle of the text

    Frontend Mentor | Recipe page

    2
    Agata Liberska•4,075
    @AgataLiberska

    Hi, the styling options for ::marker are quite limited, if you want it aligned vertically with the text, I'd just remove it and add a ::before instead, and use flex etc.

    Wondering why there's an empty paragraph at the bottom, after the article?

  • uptown_girl•370
    @uptowngirl757
    What are you most proud of, and what would you do differently next time?

    I'm glad I was able to troubleshoot and fix the issue with the disappearing SVG background while positioning the image. It took some research, but thanks to the internet, I found the solution!

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

    I'd love a clear explanation on how to make an <hr> really thin—height: 1px doesn’t seem to be enough. Also, I need a better understanding of positioning SVGs. The SVG in my background doesn’t quite match the target design. How can I achieve the exact look I want?

    Product preview card using SCSS

    1
    Agata Liberska•4,075
    @AgataLiberska

    Hi, the card looks great, but I think rather than having the .profile class on main, it would be good to use a semantic element like article for the card itself, and just use main as the container. I see you're using article for the stats, I think this could just be a ul with list items. Article would be used as a container for a content piece that is a thing on its own, and the profile stats don't make sense outside of the content of the whole card. Also be careful about section tags, they mostly make sense in slightly larger projects. Think of them like sections of the page. For example, when you're looking at this page, the whole "community feedback" could be a section, it has a heading and some content. That's not necessarily the only use but I think it makes more sense that way :) Also sections should always have headings.

    To answer your questions, you would need to use background-repeat: no-repeat and then background-position to push the circles into the two corners. As you're using two background-images, you can pass separate position values, just separate them with a comma like background-position: -50% -50%, 20% 30%;

    And for the hr,I'd just lower the opacity on it, that should make it look like it's a bit thinner :)

    Marked as helpful
  • Agata Liberska•4,075
    @AgataLiberska

    Hi Ricardo, visually your solution is nice, doesn't overflow on smaller viewports which is probably the most likely issue on cards like this. However, you could definitely simplify your html here, at the moment pretty much every element like a heading or a paragraph is wrapped in a separate div and that's really not necessary. You're also missing the <main> landmark and the card could really be an <article>, that's the element that components like this tend to match the best.

    For your scores, you're using input type="button" which also isn't the best option. Buttons and button type inputs should be used to perform an action, for example favouriting a page, or adding a bookmark. Here, we're selecting one of 5 options - and if you have a number of options and have to only select one, this really calls for radio inputs.

    I would also treat this as a form and use a submit event to update the selected score.

    Hope this helps :)

    Marked as helpful
  • Agata Liberska•4,075
    @AgataLiberska

    Hi Mario, page is looking really nice on desktop but something is off on mobile - on some screens the image looks to be covering up the heading. I think it would be a lot simpler if you didn't try to position it absolutely - have you tried just adding it with an img tag instead?

    The background image also is repeating which I'm not sure is the case in the original design.

    Nice work with form validation - I think you could maybe try to add validation as person starts correcting the email address? could be an extra challenge :) and as for preventDefault, it just prevents form from submitting to the page specified in the action attribute via the method specified in the method attribute. If you use this, you would need to provide an alternative way of handling the submission but don't worry about that for now. You can just set the input value to "" and display a success message to give it some pizzaz :)

    Marked as helpful
  • Agata Liberska•4,075
    @AgataLiberska

    Hi! Looking good and working just fine :) Definitely check out the accessibility report - the <main> landmark is missing, and the button we're using to update needs some type of labelling to be accessible. I'd also advise to add width and a height whenever you add an img - to avoid layout shifts.

    One probably very nitpicky thing - when I reload the card, the button and the quote marks keep moving up - this only lasts a fraction of a second but can look a bit like a glitch. I would suggest setting up a min-height for your advice-txt container so there always is some space there - even if it's just the height of a single line of text (I'd personally go for two lines and center vertically if it's just one line but that's your call). This layout shift wouldn't really be an issue because it happens after a user interaction but I think with some min height it would look more polished.

    Hope this helps :)

    Marked as helpful
  • Agata Liberska•4,075
    @AgataLiberska

    Hi! Nice work on this solution, visually I think this looks really nice. I would just work on the html - for a simple page like this, you're using quite a lot of nested divs which I personally think are unnecessary here, a lot of those could just be removed with no effect on the page. If you have a few block elements you don't really need to wrap them in a div unless you're grouping them for styling purposes etc. :)

    Nice work using a list for the social icons in the footer (those will need some labels/titles to validate though) but I think the three divs in 'we're different' section also should be an unordered list. And the mobile menu toggle should definitely be a button :)

    Hope this helps !

    Marked as helpful
  • Agata Liberska•4,075
    @AgataLiberska

    Hello :)

    I think you made this a bit more difficult for yourself going for buttons with ratings where this really is a small form with 5 radio inputs - So I would visually hide the inputs themselves (key word is visually , display:none wouldn't be a great choice) and style the labels to be the circles you click and then pass this to the submit event :)

    And if you want to change the look of the buttons after pressing one, loop or forEach is an ok choice, but I would simplify your code by having an 'active' class and then just using classList.toggle('active')

    hope this helps :)

    Marked as helpful
  • Agata Liberska•4,075
    @AgataLiberska

    Hi Shahid! Card is looking nice :)

    One thing I would advise is to simply start with the mobile design instead of using max width media queries here. It's simply easier (mobile designs are usually simpler) and allows for fewer lines of css. For example on mobile, the image sits on top of the text container - which you achieve by setting flex-direction:column but if you started from mobile and added display flex at 400px and wider, this wouldn't have ben necessary at all because those elements just sit on top of each other by default.

    Another thing is that 378px is not really enough space for the card to display nicely, so I would move that query up to 400, maybe 450px?

    And lastly, for accessiblity, the image of the perfume definitely needs and alt text and you're also missing a <main> landmark :) and for more semantic html, <article> tag is perfect for cards like this

    Hope this helps :)

  • Agata Liberska•4,075
    @AgataLiberska

    Hi Nader, I think the background in this challenge is particularly tricky! I would have two images added in rather than just the one, set it to no repeat and then position each individually - or just add them in with the html, might be easier to position that way :)

    For the profile image, I would probably move it to your bottom div, so it's right at the top and then move it up using tranform:translateY property :)

    Other than that, I would have another look at what tags youre using here. <article> is a perfect tag to use for card components like this, and the stats should really be a list - and you definitely don't need to put each bit of text in it's separate div. I'd suggest something like this:

    <ul>
<li><span>80K</span> followers</li>

    And if I could give one more bit of advice, try to use class names that are a bit more descriptive. I think "profile" is good, "profile-card" would be better, and then instead of top/bottom something like cover or banner and profile-info? It will make your work a lot easier on bigger challenges! :)

    Hope this helps, happy coding!

    Marked as helpful
  • Agata Liberska•4,075
    @AgataLiberska

    Hi Eslam, I think it would be a good idea to review what html tags you're using here. It looks like you're using heading tags ust based on the styles you can see which isn't the best approach. Think about hierarchy here - it makes sense for the name of the perfume to be the main heading, but all other text could just be inside <p> tags or <span>s.

    I would also rethink your button. You've added hover styles to your green div which look nice but it would suggest to me that I can click this and something would happen - but only the text is an actual link. So Instead of using a div, you could just have an anchor tag and style it to look like a button:

    <a href="/"><img src="..."> Add to Cart</a>

    Make sure to take a look at the report above as well - the image urls should use / rather than \ and make sure to use landmark elements, <main> is definitely missing from here. I would also use <article> tag for the card.

    Also, for the perfume image I think adding a good alt text would also be very important, as this isn't a purely decorative image, it shows a product we're trying to sell.

    I also had a look at your css - I think in a simple project like this there is no need to combine your selectors, as you've only really got one thing with the class price or cart. Personally I think combining selectors really needs to be thought through as in a bigger project you may run into specificity conflicts and end up with a bunch of !importants which just isn't fun to work with :)

    Hope this help, happy coding!

  • Agata Liberska•4,075
    @AgataLiberska

    Hi! I think setting those flex styles on a container class is definitely a good idea. It makes it more reusable and if you were to add something else in the body, it wouldn't be affected. You could also add justify-content:center to your styles there to center horizontally - in your solution this is done with margin:auto which also works fine, but in a bigger project if you wanted to reuse this, I think I would prefer all centering styles to be under one class (rather than a container and the component itself if that makes sense).

    Another way to achieve the same result is to use display:grid;place-items:center; - just a matter of preference which one you go for.

    What I would like to see in your solution is some more semantic tags. so your container could be a <main> and then for the card itself, I would add an <article> tag inside the container. Also don't forget to add a width and height to your image :)

    If we're also looking at accessibility, it might be a good idea to add a link around the QR image :)

    Marked as helpful
  • Agata Liberska•4,075
    @AgataLiberska

    Hi Imad!

    So there needs to be some improvements here when it comes to matching the design - you have a min height set on the cards, which stretches them unnecessarily. In general with components like this there is no need to set the height like this, instead I would control the size of the element with the content and some padding. Other than that I think it looks nice :)

    With the checkbox input though - first of all it really does need to either have a label or at least an aria-label. Also, I think it's working the other way round? The price for monthly shows up as 249, and annually as 24.99 :D no worries, it's an easy mistake to make when you're focusing on how to build something you've never done before!

    Anyway, I'm not sure that a checkbox here is the best option - I know that it is probably the most common design pattern with toggles like this so I don't blame you for choosing it! But you're really choosing between two options here (instead of choosing to have something included or not) - which really calls for radio inputs to be used. A checkbox would be for something optional which you can choose or ignore, which isn't the case here. I've got one or two projects which have a toggle built on two or three radio buttons if you want to take a look, but I am also certain that Grace Snow has done this exact challenge in a beautiful way (I know because I totally used it as a cheat sheet when building my solutions :D) oh here it is

    Anyway hope you find this helpful, happy coding! :D

    Marked as helpful
  • Agata Liberska•4,075
    @AgataLiberska

    Hi Chris! Nice work, your solution looks just like the design, and it's good to see some semantic elements :) One advice I would give is that it's a good idea to get in the habit of including width and height on your img elements to avoid any shifts in layout - that's something that google would pick up on in audits and score you down for. I would also link to google fonts in your <head> element in your stylesheet rather than importing it from css - that way it loads at the same time as the stylesheet - right now it will only download after the browser is done downloading and parsing your stylesheet. Doesn't make much difference when files are tiny but when your code base grows, it can have an effect on loading :)

    Marked as helpful
  • Agata Liberska•4,075
    @AgataLiberska

    Hi Lucas, nice work on styling here - your solution looks great. There are some overflow issues on smaller devices which you may want to look into though, and I'd definitely modify your media query - you've set the width of the card to 800px, but making the switch from mobile to desktop at 700px - so not quite enough space for the card.

    I would also recommend to look at accessibility here - a person using keyboard to navigate your solution wouldn't be able to toggle any of the accordions - the toggle should definitely be a focusable element, like a button.

    I would also like to be able to open more than one accordion at a time :)

    Hope this helps!

    Marked as helpful
  • Agata Liberska•4,075
    @AgataLiberska

    Hi Mohamed, nice work :) Awesome that you remembered to include you landmark elements :)

    However, it would be a good idea for you to really consider why you're applying certain styles. For example, you've set your body to display: flex; but you're not actually taking advantage of this - in fact, it actually messes things up a bit - when I open your solution, the card is stuck to the left of the screen - because of that display: flex property. You've also set a blue background on all elements, which you then had to override on your content container - it would've been a lot simpler to set the blue background on the body and white background on the card. Setting the height property on your card is also quite unnecessary - and again, it does mess things up on some larger mobile viewports where the white card is tiny and all content is overflowing it. Let the content take up the space that it needs, and space it out with some padding.

    Hope this helps, happy coding :)

    Marked as helpful