Skip to content
OverviewSolutions
10
Comments
14

SutonToch

@SutonToch340 points

I’m a mysterious individual who has yet to fill out my bio. One thing’s for certain: I love writing front-end code!

Latest solutions

View more solutions

Latest comments

  • SutonToch•340
    @SutonToch

    Congrats on completing the challenge!

    I really like your button, and how it pops out a little when you hover over it. Very nice.

    I think your mobile-design needs a little work, because the .img-container doesn't look right yet. It is probably because the height of your .img-container is static while the width is dynamic. Maybe try something like: min-height: 40%.

    Also, switching to mobile with a 768px breakpoint is a little earlier for such a small component. Maybe everything below 600px is fine.

    Other than that, I've never seen font-size: clamp(2rem, 4vw, 3rem); before. Fascinating, and definitely something I'm going to read up on now. Thank you!

    You did a great job. Be proud of yourself and keep on going! :)

  • SutonToch•340
    @SutonToch

    Congrats on completing the challenge!

    To remove the blue dot in Firefox, try outline: none; on the inputs. appearance: none; doesn't seem to cover outline, but I didn't know that too until now.

    There are many different ways to accomplish the rating, and choosing the radio input is definitely a good one, because you need less JS. I chose simple div's at the time, but radio inputs are better.

    I don't know how you would get the numbers into the radio input, except with awkward positioning which I'd always avoid if possible, without removing the default styles on the input. Therefore, I think using the label is the best option. But I'm also interested in other opinions :)

    Be proud of yourself for submitting the challenge to the public! You did a great job! Keep on going :)

    Marked as helpful
  • SutonToch•340
    @SutonToch

    Hello there, congrats on submitting the challenge.

    Now, there is a lot to fix. First off: don't use position: absolute; on everything. It's useful if you want to move for example an image outside of its box, but nothing else. What you effectively do with position: absolute; is to remove the natural responsiveness of your webpage. You should use flexbox and/or grid. If you want to learn more about responsive layouts, I can personally recommend 'Conquering Responsive Layouts' by Kevin Powell. It's a free course and helped me a lot in the beginnings of my journey.

    Next, take your time. To be blunt, your submission is not even close to what it should be. If you need many more hours to make it look like the design, that's fine, but do it. Quality at your stage is much more important than quantity.

    What to fix:

    • Responsiveness: flexbox/grid instead of position: absolute;
    • vertically center your component: also flexbox/grid
    • correct font-color
    • letter-spacing on 'PERFUME'
    • heading and price shouldn't be italic
    • border on the button
    • use Semantic HTML (as in, the correct tags)

    To your questions:

    • use desktop-image for mobile? → Not a good idea. Mobile devices have to load a much bigger picture than what's necessary. If you have big webpages with many more images and a slow internet connection, it will be noticeable and impact user experience.
    • what breakpoints for media queries? → Well.. there is no clear-cut answer here. Most people I've seen use the exact pixel values for specific devices. But there is a beautiful post on freecodecamp.org by David Gilbertson (https://www.freecodecamp.org/news/the-100-correct-way-to-do-css-breakpoints-88d6a5ba1862/) that actually suggests breakpoints like 600px, 900px, 1200px and 1800px. Therefore, your 600px are not wrong.

    I'm very well aware that the Quality-to-Like-Ratio looks like a troll post. If so, congrats on wasting time. If not, good things take time, and that's true for everyone.

    If you have any further questions, feel free to throw them my way.

    Marked as helpful
  • SutonToch•340
    @SutonToch

    I didn't think I'd come back so soon, but it didn't let me rest, so here we are.

    I only did very light reading on jQuery, so take my notes with a grain of salt. But my notes will most definitely make your code more readable.

    So let's get cracking:

    • Your first and second if-clause repeat each other, in that both want to close the currently open dropdown. In those cases, it's always best to create a function that handles that part for you, e.g.:
    function closeDropdown(activeDropdown) {
  $(activeDropdown).removeClass("active-dropdown");
  $(activeDropdown)
    .find(".container_right_dropdown_arrow")
    .removeClass("rotate-arrow");
  allTexts.each(function (i) {
     $(allTexts[i]).hide(300);
  });
}
    • You could do the same thing for opening a dropdown, just to stay consistent. E.g.:
    function openDropdown(clickedDropdown) {
  $(clickedDropdown).addClass("active-dropdown");
  $(clickedDropdown)
    .find(".container_right_dropdown_arrow")
    .addClass("rotate-arrow");
  allTexts.each(function (i) {
    if ($(allTexts[i].parentElement).hasClass("active-dropdown")) {
      $(allTexts[i]).show(300);
    }
  });
}
    • Let's tackle the allTexts.each(...) next. With that you loop through every single text, to find the correct one, and then show/hide it. We don't need to loop everything, because we already have the parent. To show the text when opening, you could write the following instead:
    $(clickedDropdown).find(".container_right_dropdown-text").show(300);
    • The great thing is, you already used the .find() method for the arrow, so we're staying consistent. Another bonus, we don't need the global constant allTexts anymore. You can do something very similar with closing the texts too.
    • Let's address your second if-clause next, where I can only assume that with $(dropdown).hasClass("active-dropdown") jQuery goes through every single .container_right_dropdown_each-faq and looks if it is the active dropdown. It would be more efficient if we would store the active dropdown in a global variable. This way we could close it directly using closeDropdown(activeDropdown).
    • Obviously, a few tweaks are needed for that to work. When opening a dropdown, that clicked dropdown must be saved to the active dropdown. When closing a dropdown, the active dropdown must be removed (e.g. reassign with null).
    • In your first if-clause, we can now also check if the clickedDropdown is our activeDropdown via $(clickedDropdown).is(activeDropdown).
    • Now our internal logic when a dropdown element is clicked looks more like this:
    dropdown.click(function (e) {
  // saves the element clicked in a variable
  let clickedDropdown = e.currentTarget;

  if ($(clickedDropdown).is(activeDropdown)) {
    // Clicked on the only open dropdown
    closeDropdown(clickedDropdown);
  } else {
    // Clicked on a different dropdown, than the open one -> close the original open one
    closeDropdown(activeDropdown);

    // Open the clicked dropdown
    openDropdown(clickedDropdown)
  }
});
    • Puh.. this was probably a lot to take in. I'd recommend retracing my written steps in your own pace again.

    A few takeaways:

    • Think about what functionalities you want to implement (e.g. opening and closing a dropdown) and write a function for those.
    • Then create your logic, that calls the functions with the correct parameters when they are needed.
    • Make use of the information you already have (e.g. the clicked element and therefore the text of its child)
    • Usually that's an iterative process, because it's very rare that the original plan pans out exactly like one hoped, or you find an even more efficient way.

    I hope I was able to help you with this :), but don't forget that it's very normal for the first iteration of code to "just work". I'm pretty sure my proposed improvements can be improved even further. Feel free to ask me any further questions, I'm always happy to help.

    Keep on going!

    Marked as helpful
  • SutonToch•340
    @SutonToch

    Congrats! I'm glad that this challenge gave you some trouble :P because I had a lot of trouble.

    Again, I really like your comments, makes everything so much more readable. I need to do that too.

    Your positioned elements look fine to me. I can't really comment on the js, because I still need to learn jQuery, but it looks good, for whatever that's worth. I'll come back to it, though.

    Small notes:

    • Currently, if you visited the webpage on mobile, the desktop images are loaded too. With this few images, it doesn't really make any difference, unless you have a really slow internet connection. To solve that, you could take a look at the <picture>-element.
    • Maybe consider adding a transition: transform 0.3s; for your arrow-icons, but that's a style choice and not really needed because of the really nice show and hide animations of the text.
    • I don't think transition: 0.3s; on .container_right_dropdown does anything. Probably an artifact before you added the animation via jQuery.
    • Consider adding :focus to your :hover element too, for people that tab through webpages.

    In terms of styling, the font-size could be a little bigger and the box-shadow a little more prominent. Other than that, it's perfect.

    Well done! This is a really nice solution. And I'll definitely come back for the jQuery :).

    Marked as helpful
  • SutonToch•340
    @SutonToch

    Congrats on completing the challenge!

    I really like your Semantic HTML and clear naming.

    In terms of design, I like your changes to background-color and the more saturated rows :) it just fits. I'd just recommend giving your elements a little more white-space to breath, especially the rows are a little squished right now.

    To your question, how to center .score:

    • I'm guessing you tried margin: 0 auto; since you used margin: 0 27%;. The auto keyword basically tells the browser to just deal with the exact number itself. Since <section> is a block-element, it has width: 100%;. Therefore, the browser decides, no margin is needed, because there is no whitespace left. Now, if you give the circle an absolute width equal to your height, the .score box doesn't take up the entire horizontal space anymore and the browser can center the box using margin: 0 auto;.
    • Just remember: for margin: 0 auto; to work, the element needs to be a block-element and it needs a width.
    • In the spirit of flexbox, another solution could have been to turn the entire .result into a flex-column and use align-items: center;. Half of that you already did in your .summary.
    • Yet another solution is to turn the .score into a inline-block using display: inline-block. This way text-align: center; applies to .score as well, since text-align does not only align text, but all inline-elements.

    Some nitpicking:

    • align-self: center; in .score and btn doesn't do anything because it's not a flex/grid-element.
    • <btn> is not the correct html tag for buttons (<button>). While it works, similar to Semantic HTML, it's important for screenreader and SEO to use the correct tags. 'btn' is usually the abbreviation used for the button class-name, so that's probably why you confused the two.
    • flex-direction: row; in .container doesn't do anything, because it's the default value. I guess you thought that the flex-direction: column; is passed down to the .container. Usually only typography (e.g. font-size, color, font-family...) is passed down to the children.
    • transition-duration: 0.4s; on its own in your btn doesn't do anything. CSS also needs to know which property needs a transition, in your case background-image. If you want to know more about transitions, check out MDN (https://developer.mozilla.org/en-US/docs/Web/CSS/transition).
    • Your media query is beautifully minimalistic. In mobile, you shouldn't center your component anymore → remove those properties from your body-tag.
    • as @Mahmoudamin11 already said: cursor: pointer; on the button would be nice. But changing <btn> to <button> would fix that too.

    As a general tip, try checking your browser dev-tools for any unnecessary CSS. This way you have fewer lines in CSS, which makes everything more readable.

    Everything else is looking good, :) if you want to further improve, I'd recommend checking out CSS-Variables (https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties) next.

    Thanks to your contribution I learned, that linear-gradient() is basically an image, and instead of background you can also use background-image. Who would have thought.

    Be proud of yourself for completing the challenge and submitting it!

    Keep on going and you'll be on a good path :)

    Marked as helpful
View more comments