Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

All comments

  • gayathri-v1• 170

    @gayathri-v1

    Submitted

    What are you most proud of, and what would you do differently next time?

    I was able to code JS. I learnt about onclick, simle fucntion and calling a function in JS. Media queries in JS.

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

    I tried max possible to imitate the preview I took lot of time to complete this, I did not add share button in active state in mobile view. Other than that its good overall.

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

    I'm open to suggestions

    Maan Al-Hababi• 190

    @MaanAlHababi

    Posted

    Hey there!

    Congratulations on completing this challenge. There are a few things I'd like to point out. First of all, your HTML could do with a bit of semantic restructuring. Rather than using a lot of <div> elements, use section elements, buttons, articles and others. You should be familiar with a wide range of elements despite how similar their purpose is, or whether you could have the same effect of a button with a div element and an onclick attribute. To understand better why this is important I recommend reading about semantic HTML.

    Secondly, you could make your button trigger on & off rather than on and once and permanently on. This can be done with an element's classList and the toggle method of the classList object. Every element in the DOM has a list of classes (you are probably familiar with the fact that you can add multiple class attributes to an element separated by spaces.) If you assign an element to a variable in JS - using getElementById, or querySelector or whatever (also it is best practice to declare variables that store elements using const rather than var):

    const elementVariable = document.getElementById('element-id');
    

    This element's classList is a property that can be accessed through:

    elementVariable.classList
    

    You can use this to your advantage and add a selector in your CSS for a 'hidden' class:

    .hidden {
        display: none;
    }
    

    Then, you can utilize the classList's toggle method to add/remove the 'hidden' class from the 'share menu' on the click of a button. (The toggle method adds the class if it's not in the classList and removes it if it is there, by calling this method on the click of a button you can toggle/alternate at any point in time):

    const myButton = document.getElementById('my-button') //<button> element;
    const myMenu = document.getElementById('my-menu');
    
    myButton.addEventListener('click' () => {
        myMenu.classList.toggle('hidden');
    })
    

    Marked as helpful

    1
  • Teezee86• 100

    @Teezee86

    Submitted

    What are you most proud of, and what would you do differently next time?

    making it responsive across browsers

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

    Still having trouble with font sizes in firefox

    Maan Al-Hababi• 190

    @MaanAlHababi

    Posted

    Hey, Teezee86!

    Great job for completing this challenge. Might I bring to your attention the fact that at mobile size, the grid seems to be dislodged upwards where a lot of content is not visible. This is caused by setting height properties to the html and body tags:

    html {
        height: 100vh;
    }
    
    body {
      height: 100%;
    }
    

    Setting a height to the html tag is pretty much redundant, this way you're explicitly limiting the size of your page when you can simply do that by styling your content neatly, specifically being aware of overflowing content that causes breakages. In summary, adding a height tag is useless, and in this case, it has caused trouble.

    Simply removing these properties should do the trick. Similarly, adding a width and max-width properties to the body tags is also redundant. Keep in mind, setting the max-width to a percentage value is meaningless since more often than not it does not actually give you the desired effect.

    Removing this part of your code should fix the breakage I mentioned, removes redundant lines of code, and sets you up to manage aligning your content and using up the space available to you better.

    html {
        height: 100vh;
    }
    
    body {
        height: 100%;
        max-width: 100%;
        width: 100%;
    }
    

    Happy coding!

    Marked as helpful

    1
  • Maan Al-Hababi• 190

    @MaanAlHababi

    Posted

    Hey, Anton!

    Nice work recreating this project, but allow me to point out that you overlooked the mobile design. A major part of the project is building a responsive web application. Which means, your website should adapt to different screen sizes.

    I recommend you go through the responsive web design learning path here on Frontend Mentor, but here is a brief insight. A convenient approach is going Mobile-first which means you start by designing the website for mobile devices since usually websites are simpler on smaller screens. Then, add complexity as the size of the screen increases which can be established through media queries.

    In your code, you explicitly set a size for the website:

    body {
        max-width: 1440px;
        height: 150vh;
    }
    

    This way only a select number of users get to enjoy the best experience on your website, users with larger screen sizes see the content centered relative to this max-width, while the page as a whole is actually aligned to the left.

    Finally, it is best not to set a 'hard-constraint' height on the page. In your case this just created a bunch of unnecessary white space beneath the main content of your website.

    Marked as helpful

    0
  • Maan Al-Hababi• 190

    @MaanAlHababi

    Posted

    Hey there!

    In your code, you "hardcoded" some styling that could be applied in different situations in a real-world project. Specifically, I'm pointing out the 'PERFUME' text.

    <p>P E R F U M E</p>
    

    You explicitly made the text uppercase, and added spaces between the characters. It's an important skill in front-end development to be flexible with the styling. In a real-world situation you might have different categories:

    1. Perfume
    2. Accessory
    3. Hat etc.. This approach would end up awkward for the user and the developer. My personal approach was to use the text-transform: uppercase property for this element. So, the case of the text in the code does not matter since at the end, the CSS will transform the text to uppercase. Secondly, you can use the letter-spacing: *value* property, which establishes this effect for you and gives you more control over the degree of spacing in the text.

    This way, you can also improve readability of the code.

    
    
    <p class="product">perfume</p>
    

    Marked as helpful

    1