Skip to content
OverviewSolutions
36
Comments
11
Florian Stăncioiu
@florianstancioiu

All comments

  • Florian Stăncioiu•710
    @florianstancioiu

    I managed to get the Storybook published on Chromatic.

  • Florian Stăncioiu•710
    @florianstancioiu

    I updated my solution so that a new advice would load on mouse click.

    The reason why the click seemed to not trigger a HTTP request was because of HTTP caching, I had to cache bust every fetch request. The click was triggering a request, except the data from that request was always the same because of caching...

  • Florian Stăncioiu•710
    @florianstancioiu

    Hi Laura,

    Here is my advice on the eye section of the challenge:

    You can overwrite the existing .overlay div with this HTML structure:

    <div class="overlay-wrapper">
<div class="overlay"></div>
<img class="icon" src="" alt="Eye Icon" />
</div>

    Don't keep the eye icon inside the .overlay div because the opacity property affects every single child element. Use the z-index property to get the .icon in front of the .overlay.

    Note: The z-index property only works if there's a position property on the element as well.

    Here is some advice that will help you write better CSS code overall:

    • Install prettier in Visual Studio Code (I asume you use vscode). Follow a youtube tutorial if you can't get the hang of it with the written text, prettier is a must have for a beginner.

    • Use rem instead of px, here is a video about it. And here is how to use rem in a nutshell: you set the font-size of the html tag to 62.5% which makes 1rem equal to 10px for all child elements

    html { font-size: 62.5%; } 
body { font-size: 1.4rem; } /* =14px */
h1   { font-size: 2.4rem; } /* =24px */
    • Instead of doing this in CSS:
    * {
margin: 0;
padding: 0;
box-sizing: border-box;
}

    Use a modern CSS reset like this one here.

    Marked as helpful
  • Florian Stăncioiu•710
    @florianstancioiu

    Well done, the new design looks fantastic!

    Great work, I'm speechless!

  • Florian Stăncioiu•710
    @florianstancioiu

    Hi Mubaraq,

    Here are some tips to make your code more readable.

    • First off, install prettier for vscode (I'm assuming you use Visual Studio Code, if not, please install prettier for your text editor of choice). https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode

    • Secondly, try to separate the JavaScript logic in different files (You can create a slider.js for the slider and a cart.js file for the cart), writting all JS code inside one giant file is a big mistake and should be avoided.

    • Thirdly, writting HTML strings directly to DOM is prone to security exploits like XSS. You can aleviate this by using <template></template> tags or by using a template engine. Here are some resources on using the template tag: https://javascript.info/template-element , https://www.w3schools.com/tags/tag_template.asp

    (Don't lose too much sleep over the third tip)

    Now let's address your issues:

    1. Updating the total cost price- I get a nan on the total cost price of the product

    The reason why you get NaN is because the string starts with a $, and that turns into NaN. The simplest solution is to remove the $ character using productPrice.substring(1).

    // If the product is not in the cart, add it as a new cart item
const cartItem = {
  // ...
  totalPrice: Number(productPrice.substring(1)) * Number(productCount),
};
    1. Deleting a product from the cart - the remove button doesn't seem to be triggered. Was working fine before

    The reason why it didn't work is because the click event is targeting the image element, and the image element doesnt have the .remove-button class, so we have to look for the nearest/closest element with that class.

    To fix it, replace this line:

    if (event.target.classList.contains('remove-button')) {
  // ...
}

    with

    if (event.target.closest('.remove-button')) {
  // ...
  // And comment this function call bellow, otherwise you will get an error:
  // updateCartTotalPrice()
}

    As a last note, don't create functions inside event handlers, create the functions in the global scope. Also, don't create event listeners inside event listeners.

    // BAD
addToCartButton.addEventListener('click', () => {
  function updateCartItemCount() {}
  function updateCartItem(cartItem) {}
  function updateCartTotalPrice() {}

  cartIcon.addEventListener('click', () => {});
});

    // GOOD
addToCartButton.addEventListener('click', () => {});

function updateCartItemCount() {}
function updateCartItem(cartItem) {}
function updateCartTotalPrice() {}

cartIcon.addEventListener('click', () => {});

    I think you will still face some issues after tweaking the code using my advice, especially with the cart. Please don't take my advice the wrong way, I'm only trying to help you out. I hope this will help you a bit in your journey, keep on coding!

    Florian

    Marked as helpful
  • Florian Stăncioiu•710
    @florianstancioiu

    Hi there,

    As I mentioned in the solution description, I revisited the challenge and updated the github repo with scroll triggered animations. I followed David Omotayo's article on LogRocket.com about Framer Motion. In that article, he suggest using a different library for detecting if an element is in viewport, I didn't went down that path, I realized that Framer Motion has it's own built-in useInView hook that does the same thing as the library he suggested, and in the end, I used only Framer Motion.

    That's all, Florian

  • Florian Stăncioiu•710
    @florianstancioiu

    Hi Ric,

    You shouldn't have 2 index files, and localstorage shouldn't be used in this project. You can take a look at how I did the challenge here: https://www.youtube.com/watch?v=oZJb_GPAlc4 The JavaScript part is done at the end of the video, you can also find the code in the description of the video.

    Marked as helpful
  • Florian Stăncioiu•710
    @florianstancioiu

    Hi Zana,

    I personally like to use the <link> tag in the head section to request the fonts, I don't like my CSS file to trigger any requests instead I want to see them all in one place, in HTML, I think it's more intuitive this way, but there isn't a best practice regarding this, you can do as you like.

    As for rem, I also use html { font-size: 62.5%; }, I recently discovered it on my own (even though I have years of development behind me...). I think it's an awesome way of creating UIs and making them accessible while retaining developer friendliness. Where did you saw that rem shouldn't be used? I want to learn more about this too.

    Kindly, Florian

    Marked as helpful
  • Florian Stăncioiu•710
    @florianstancioiu

    Apparently, I forgot to add that pesky warning sign when there's an error on an input. :(

  • Florian Stăncioiu•710
    @florianstancioiu

    Hi,

    I don't think there's a better way of adding the stars (there is another way of adding the images that would not involve writing them multiple times but the click functionality would be lost and without it, the ratings are useless).

    As for the ratings, I used position: relative and set the left property to a negative value. The ratings could have been done also using a negative margin but I think the way you did it with translate is probably the most elegant way.

    Keep on coding!

    Marked as helpful
  • Florian Stăncioiu•710
    @florianstancioiu

    Hi Nick,

    You can actually use background-image with multiple images on the body or the containing div you have and after you do that you can position each individual image using background-position. Make sure to disable the repeating backgrounds using background-repeat: no-repeat;. You don't need to have 2 extra divs for the background images, it might work that way though but I haven't tried it.