Skip to content
OverviewSolutions
10
Comments
10
Luiza Uszacki
@LuizaUszacki

All comments

  • Nicolas Eiriz•50
    @NicolasEirizSubmitted over 2 years ago

    Hi guys, for this challenge I started using media queries, please if you have any comment/suggestion let me know.

    Thanks!

    Single price grid component

    1
    Luiza Uszacki•250
    @LuizaUszackiPosted over 2 years ago

    Hi @NicolasEiriz. Congratulations on the project! To be honest, I'm impressed with how accurate it looks.

    I just wanted to point out that depending on the screen's size, your words will jump out of their container and stay above other words. You can see it if you use the "Dimensions" on DevTools.

    Marked as helpful
    0
  • Luiza Uszacki•250
    @LuizaUszackiPosted over 2 years ago

    Hi @korede1004, good work completing the challenge!

    I also have some tips for you:

    1. Instead of using img tags to put the images as the background you could use the background-image: url('./images/bg-pattern-bottom.svg'). You can even add more than one image and the background color to the same element.

    2. Think about adding the main tag to your HTML for accessibility purposes and wrapping the main content in it.

    3. I saw that you have a padding-top to center the image, but depending on the screen size, it won't stay in the center. So, you could use flexbox. Once defined the container of the element, add display: flex; align-items: center; justify-content: center; on this container and it will center all its direct children in the center on both axes.

    Just an extra tip: you could use your body as the background/container of the card and build your card in the main tag.

    Hope it helps!

    Marked as helpful
    0
  • Luiza Uszacki•250
    @LuizaUszackiPosted over 2 years ago

    Congratulations on your project, @vonsdesign!

    Just a little tip, try to add cursor: pointer on your button to show it's clickable (it will show a hand when you hover over it).

    Marked as helpful
    1
  • Luiza Uszacki•250
    @LuizaUszackiPosted over 2 years ago

    Hi, @RalphJnr.

    The background-image isn't showing because your "styles.css" is inside a folder. So, you have to add ../ at the beginning of your path (url) to go back a directory.

    The way you're doing it, it's trying to find a images/pattern-background-desktop.svg inside the folder "css".

    Marked as helpful
    0
  • Mohamed Metwally•50
    @mmetwally0Submitted over 2 years ago

    Would definitely love to hear feedback about the button's shadow & ways to improve it, and the design overall. Thanks in advance!

    Order summary component

    3
    Luiza Uszacki•250
    @LuizaUszackiPosted over 2 years ago

    Hi, @mmetwally0.

    Instead of main .card-container .summary-box .payment a:hover try main .card-container .summary-box .payment > a:hover. It will change the color just of the payment's a direct child. So, you won't have your payment-btn's a changing its text color.

    You could also add a transition to make the change smoother.

    1
  • Luiza Uszacki•250
    @LuizaUszackiPosted over 2 years ago

    Hi @joopWU.

    I saw your body doesn't have a height. Therefore, its height is the same as the container. So, to make it have all the screen's height as the height you might want to add min-height: 100vh. With that, your body will have at least all the screen's height as its height. After that, you can add the background-color.

    Doing that, your screen will seem a little bigger than the screen's height, tho. It's because, by default, some elements have a margin, and your body has a default margin of 8px on each side. So it's the screen's height plus 16px (8px in the top and 8px in the bottom). You could reset this with body { margin: 0;} or you could apply it to all elements using *, it's up to you.

    I tried doing that in your project with dev tools and it created a new issue. Your container will be stuck in the body's top and will add a margin to the top of the body instead of between the body and the container. If you want to know more about it you can click on this link Everything You Need To Know About CSS Margins. The "PARENT AND FIRST OR LAST CHILD ELEMENT" part shows exactly what will happen to your project and how to solve it.

    So... If you add something like border: 1px solid transparent; it might solve it. But it will make the body's height bigger than the screen's size again. It's because the height will be 100vh + 2px (1px each side). To avoid that you can set box-sizing: border-box to all your elements (*) and they will have the size you set without adding anything more because of the padding or border.

    But if you don't want to run into this issue, what I highly recommend is using flexbox to center your card. (still resetting the element's margin, setting box-sizing: border-box and having the body's min-height as 100vh).

    0
  • alexeira•60
    @alexeiraSubmitted over 2 years ago

    As the challenge is made for a desktop of 1440px and a mobile of 375px, I do not take into account the other resolutions so when you enter the page depending on the browser, screen resolution and the zoom you have on it, the size of the component may look "unstylized".

    • Is it necessary to style those other resolutions for the challenge?
    • Could you take it more as a personal challenge?

    Interactive rating component

    3
    Luiza Uszacki•250
    @LuizaUszackiPosted over 2 years ago

    I think it's a good thing to make the website responsive to different screen sizes, even if it's just a challenge. I usually like to make it responsive and treat the challenge as a real project to improve my skills as a personal challenge (and also because I'm a perfectionist lol).

    I just saw the challenge and it sees like it's necessary to style the other resolutions. In the "readme-template" there is a part that says:

    The challenge

    Users should be able to:

    - View the optimal layout for the app depending on their device's screen size

    • See hover states for all interactive elements on the page
    • Select and submit a number rating
    • See the "Thank you" card state after submitting a rating

    And just one last thing... If you didn't make it responsive because you didn't know if it was mandatory, you can just skip it but if you got stuck, I can help you:

    • You can use something like 400px instead of 1440px in your @media at the main and put the width to something like 360px;
    • Another way to do it (and what I would do) is to set your card's width as width: min(360px, 100% - 64px). In this way, your card's width would always be the smallest value (a fixed value of 360px on big screens or the screen's width minus 32px (2rem) on each side for the margin on small screens).
    3
  • Luiza Uszacki•250
    @LuizaUszackiPosted over 2 years ago

    Hi @Deen-Abdultawab, congratulations on the website. I would also advise you to...

    1. Put the type of the input product-quantity as number instead of text (in that case, you would have to hide the arrows that will come with that type to have the same appearence), because now, even if I can't submit a text, I still can write there;

    2. You could also test if the product-quantity has a value greater than zero, and just if so, decrease its value. Otherwise, your input can receive negative values;

    minusBtn.addEventListener('click', () => {
  if (productQuantity.value > 0) {
    initialValue--;
    productQuantity.value = initialValue;
  }
})

    1. I also saw you put cursor: pointer on fa-solid, in that case, you could increase its padding to turn the clickable area bigger, making it easier for the user to click on it;

    2. And at least, all your main-nav has cursor: pointer not just the clickable areas.

    I hope it helps you.

    Marked as helpful
    1
  • Luiza Uszacki•250
    @LuizaUszackiPosted over 2 years ago

    Nice job completing the challenge. I hope to help you with some advice about your code.

    1. If you click on more than one number, all clicked numbers will be selected instead of just the last one. So, to avoid it, you could remove the selected class from your buttons when selecting a button (of course, it's not the only way to solve it, you could also save the first selected button and remove the class just from it). Then, it will have just one button becoming gray, even if you click on more than one button.

    Like that:

    rateBtns.forEach((btn) => {
  btn.addEventListener("click", () => {
    removeSelected();
    btn.classList.toggle("selected");     // In this way, toggle will always add the class
    yourRate = btn.innerHTML;
  });
});

function removeSelected() {
    rateBtns.forEach(btn => {
        btn.classList.remove('selected');
    }) 
}
    1. I believe you won't need the second advice if you follow the first one because you will always have at least one number selected after selecting the first one, but I will tell you anyway, just in case... The way you are doing, if you select and deselect one or more buttons and try to submit it, it'll still submit even without any button selected. To solve it, instead of using yourRate != "", you could check whether any of the buttons in rateBtns has the class selected or not.

    Hope it helps you.

    Marked as helpful
    0
  • Soojeong•220
    @kongguksuSubmitted over 2 years ago

    edited: I fixed the bug and now the codes are simplified!

    This is my first Frontend Mentor JS challenge. I'm learning JavaScript but I realized it's still quite challenging for me to adapt what I learned into actual project, even for a small one like this.

    I couldn't follow the DRY principle (Do not Repeat Yourself) and ended up writing a ton of codes that were repeated over and over again.

    Please let me know if there is a better way to fix this :)

    Interactive Rating Component

    3
    Luiza Uszacki•250
    @LuizaUszackiPosted over 2 years ago

    Nice job completing the challenge.

    I would also advise you to put cursor: pointer in your buttons to show a hand when you hover over them and show it's clickable.

    Marked as helpful
    1