Skip to content
OverviewSolutions
10
Comments
14
SutonToch
@SutonToch

All comments

  • SutonToch•340
    @SutonTochPosted about 1 year ago

    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! :)

    0
  • devusexu•130
    @devusexuSubmitted over 1 year ago

    In this challenge, I removed input style with appearance: none, using just label for rating instead since you can click respective label to select that radio button.

    My question is: How to eliminate the difference(a blue dot for checked radio button) of appearance: none between Chrome and Firefox? Here's the screenshots about this issue Chrome Firefox

    What's your solution to this challenge? Do you style the input without completely removing the default styles? Any advice/comments are welcomed🙏

    (updated: the blue dot issue would be solved by setting input outline: none, thanks to @SutonToch)

    Interactive component with appearance:none for input

    1
    SutonToch•340
    @SutonTochPosted over 1 year ago

    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
    1
  • SolidEnder•120
    @SolidEnderSubmitted over 1 year ago

    This is probably the second project I was working on initially, so I was kind of new to optimizing pages for mobile and desktop devices still. I think the biggest challenge on this one was just changing the dimensions of the product image to be able to fill in the containers for mobile and desktop view without it looking too scrunched up, and it was definitely a grind to reposition and modify the dimensions of the text and button elements. I was initially unsure of whether or not it was a good idea to use just one of the pictures included for both views, like I think I used the desktop image for both mobile and desktop views but after some tinkering I did find out that wasn't that big of a deal. One question I have about best practices I have now, even though I probably could've asked earlier- would it be better to set the min or max width to the precise pixel count of mobile or desktop devices, or is giving it some of the wiggle room perfectly fine like I've been doing? I've mostly been setting it to < or > 600px personally. Feel free to leave your suggestions or feedback with me!

    Preview Card Component(html, css)

    1
    SutonToch•340
    @SutonTochPosted over 1 year ago

    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
    1
  • OsolalDev•70
    @OdaSolaDevSubmitted over 1 year ago

    ¡Hey there! ^-^ I finally did one challenge that gave me a good amount of trouble. I would appreciate some feedback, especially about how I positioned the images and perhaps some comments about my js code.

    FAQ accordion card

    2
    SutonToch•340
    @SutonTochPosted over 1 year ago

    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
    1
  • OsolalDev•70
    @OdaSolaDevSubmitted over 1 year ago

    ¡Hey there! ^-^ I finally did one challenge that gave me a good amount of trouble. I would appreciate some feedback, especially about how I positioned the images and perhaps some comments about my js code.

    FAQ accordion card

    2
    SutonToch•340
    @SutonTochPosted over 1 year ago

    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
    0
  • SutonToch•340
    @SutonTochPosted over 1 year ago

    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
    0
  • tghezfe•60
    @tghezfeSubmitted over 1 year ago

    This is my second challenge, having some knowledge but putting it into practice. I found it interesting that there can be two ways of working with the image:

    1. As a background image of the container.

    2. Changing it for a display: none; depending on the width of the screen (this was my case or path).

    However, the difficult part in my opinion is the responsive part.

    So I'm open for tips or feedback.

    product-preview-card-component-main

    1
    SutonToch•340
    @SutonTochPosted over 1 year ago

    Congrats on completing the challenge!

    I really like your Semantic HTML and transition on the button. When I started, I haven't paid any attention to both of those things. Also, your CSS-Variables look very nice.

    Regarding your responsiveness:

    • I don't think you need the (orientation: landscape) on your second media query: (min-width: 768). Otherwise, your website will always be mobile design, as long as the width is smaller than your height. To be fair, that's rarely going to happen, but it feels a little cleaner.
    • Maybe put an max-width: 500px; instead of max-width: 90%; on your .card. Or make it 60%/70% and give it an min-width: 375px;. Otherwise, the image will be much bigger than the text in mobile at higher widths. Just try it out and see what you like more :).

    If you're still unsure about responsive design, maybe 'Conquering Responsive Layouts' by Kevin Powell might be useful to you. It's free and taught me a lot! But you seem to be much further in your HTML/CSS-Journey than I was at the time, so you have to see for yourself.

    But there is not much else to add, you did a great job! I'm sure you're ready for more difficult challenges. If you still want to stay in the 'Newbie'-area, I can recommend the 'faq-accordion-card' or 'results-summary-component' to up the difficulty.

    Keep on going!

    1
  • SutonToch•340
    @SutonTochPosted over 1 year ago

    This solution looks excellent!

    You can absolutely see the amount of care that went into this, good job!

    To your question: I don't think that you have to use then 'Frontend Mentor'-Logo for your projects. Never read anything about that, anyway. I mostly do because of convenience, but might change that in the future.

    Some general advice:

    • Use more CSS-Variables. While your design looks absolutely excellent, there are many absolute values and tiny tweaks you made. This makes maintenance very time-consuming. Having CSS-Variables for border-radius, font-size, color and spacing could reduce the necessary time + your design looks more consistent. Sometimes designers get lost in detail, but that doesn't always mean we as Frontend-Devs have to.

    Nitpicking:

    • You could have used your custom-variable --gradiente for the background in .box too
    • Use more shorthands for margin and padding, e.g. in .element: padding-left: 15px; padding-right: 15px → padding: 0 15px;. This is one line less and makes everything a little bit more readable.
    • There is some unnecessary code like: .summaryScreen {padding: 1px;}. No human would notice that.
    • Some inconsistencies with color, where sometimes you use color: hsl(0,0%,100%); and sometimes color: white;
    • In your .score-classes, you used an HTML-Tag <spam>, but I'm pretty sure you wanted to use <span>. But I would leave it because it's a funny typo xD.
    • For HTML-Semantics, you could have used <main> and <section> Tags to improve readability and SEO-ranking.

    I've never seen the @media (hover: hover) {...} before. Very interesting! Your comment states that: "this code changes the color of the button when the cursor passes over it". For that, you don't need the media-query. .continue:hover {...} would have been enough. According to (https://dev.to/jackdomleo7/media-hover-hover-css-media-query-5bge) you can use this media-query to create specific styles for devices that can hover → hover-specific-styles so to speak. But I've never used it before.

    Congrats on completing the challenge!

    This wasn't an easy "Newbie"-Challenge. Be proud of your attention to detail, very impressive!

    Marked as helpful
    1
  • SutonToch•340
    @SutonTochPosted over 1 year ago

    Congrats on completing the challenge!

    I really like:

    • Your GitHub. Your README shows that you have taken extra time to make it look nice.
    • Your JavaScript. The Solution with the event listener on the rating-container instead of on each button in conjunction with e.target.closest(".rating-button") looks really elegant. Good Job!

    Some notes on CSS:

    • You declare font-family in the body twice
    • If width and height are to be absolute, I'd recommend using pixels instead of rem. Personally, I have a better understanding of what 51px is, instead of 3.2rem. But that might differ from person to person. If it's unclear, rem looks at the root font-size (usually 16px) and is therefore the more absolute/static counterpart to em which looks at the font-size of the parent or same element (depending on if you use it for font-size or not).
    • Nitpick: font-size: 1rem and line-height: 1 doesn't do anything unless you overwrite something. I'd recommend checking your CSS in the Browser Dev-Tools for unnecessary code. Less code → more readable.

    Sadly, there is one fatal flaw:

    • You can submit without choosing a rating. Easy to fix with a conditional and judging by your JS I'm pretty sure you don't need a hint.

    Thanks to your contribution, I learned about the defer attribute of the

    Marked as helpful
    0
  • OsolalDev•70
    @OdaSolaDevSubmitted over 1 year ago

    Hey there! ^-^ I'm starting to do my own stuff since I started learning Web Development, and I would appreciate some feedback, especially about how I write my code (if it can be optimized or other ways of doing the same thing).

    Interactive rating component | jQuery

    2
    SutonToch•340
    @SutonTochPosted over 1 year ago

    Looks great! Congrats on completing the challenge.

    I really like your comments and the way you space things out. Makes everything very readable.

    Your code looks absolutely fine to me and everything works like I would expect it. Some notes:

    • You declared CSS-variables but never used them. If you want to use your --lightGrey for example, replace #959eac outside :root with var(--lightGrey).
    • Your JS-variables should be const instead of let unless you reassign them. That way, it's more clear what is going to be reassigned at first glance.
    • You have a TypeError if someone tries to submit before choosing a rating. You can probably fix that with a simple conditional in your submitRating.click(...).

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

    Be proud of yourself and keep on going!

    Marked as helpful
    0
  • saina-s•30
    @saina-sSubmitted over 1 year ago

    Sorry for the previous wrong code. I wasn't sure how to place number 76 in the circle and not still sure about that if I placed "of 100" right or not. I'll also appreciate it if you give me some feedback and advice on my media query since there is an empty part between results and browser bar. ( I really have no idea about how I should fix it)

    flexbox- hover and active pseudo

    1
    SutonToch•340
    @SutonTochPosted over 1 year ago

    Congrats on completing the challenge! Good Job!

    Addressing your Questions:

    • How to place number 76
      • You already did most of the work! Instead of, justify-content: flex-end you could use justify-content: center. After that, maybe increase the font-size of your #SeventySix and reduce the margin.
    • Media query → empty part between results and browser bar
      • margin: 0 on body. The body has a default margin of 8px, that you almost always want to override

    Feedback:

    • Style really shouldn't be in the HTML. While you mainly used it for typography, there is also typography in the .css, which makes it inconsistent.
    • You could have included the font-family via @font-face instead of Google Fonts, because this is one of the rare projects where the font was already provided.
    • Currently your container is not vertically centered. Instead of, top: 50px you could use top: 50% if you add height: 100vh to the body. Another way would be to scrap the position: relative altogether and use the following on your body: display: flex; flex-direction: column; justify-content: center; height: 100vh.
      • 100vh basically means 100% of your viewport height, no matter the screen size.
    • Your button should also have the linear-gradient on :hover, so that it is clearer for the user that it can be clicked.

    Nitpicking (and therefore not really important):

    • spacing in the .html is a little bit all over the place and makes it harder to read
      • e.g. 2 <link>-Tags are not indented in the <head>, 2 line-breaks separate the .result while .summary has 4 line-breaks at the bottom
    • also spacing the in .css (e.g. .summary and .component are more indented than .container)
    • class naming:
      • while a class like '.hundred' is very descriptive for static values, imaging making this component dynamic. You should use more general descriptions on these things, e.g. 'subscore-max' or 'score-max'
    • Your "summary"-title should be "Summary"
    • You have some unnecessary CSS. Try using the Dev-Tools in your browser (should be F12) to see if your declared styles make any difference.

    Tips:

    • Use * {box-sizing: border-box} to create a more intuitive layout. This way, padding and margin aren't increasing the box they are applied to, but instead shrink the content-size while the box-size stays the same. But I don't think that it changes anything in your case.
    • I recommend a mobile-first approach in terms of CSS. First do the HTML for the Desktop-Layout, but then the CSS for the Mobile-Layout first. This way you take advantage of display: block default behavior of many elements (such as div's) because in Mobile-Layout most elements are display: block and if not, that also applies to the Desktop-Layout. The result is less written CSS and less bulky media-queries. Just try it for yourself and see if you like it :).

    I really like:

    • your GitHub (you did the README + have multiple commits → that's rarer than you think)
    • you used a box-shadow (it could use a little more contrast tho)

    Sadly, I've limited time. If you want, I can take another look at it in the near future, or somebody else can provide further feedback. If you have any questions, feel free to throw them my way :) I'll do my best to answer them.

    Be proud of yourself! You completed this challenge AND submitted it to the public!

    I hope you learned something, and have a wonderful day :).

    Marked as helpful
    1
  • SutonToch•340
    @SutonTochPosted over 1 year ago

    Good Job!

    I really like your organized .css and clean naming convention.

    The feedback above is great, and I'd like to second that.

    The only major thing I'd like to add is, that I'd recommend a mobile-first approach in terms of styling. First do the HTML for the desktop-design, but then style for the mobile-design first. This way, you usually have to write less CSS and media queries. Because in mobile designs, many Elements take up the entire space of a line, as does a block Element, which is the default behavior of many elements such as divs. Just try it out and see if you like it :).

    Some nitpicking:

    • Add the padding shorthand on left-container, you already did it on everything else, it deserves the attention xD
    • Add a 'cursor: pointer' to the button, because it currently doesn't look like it can be clicked.
    • Use your hsl(228, 12%, 48%) for the #perfume-description as well, since you already used it for the #perfume-header

    Be proud of yourself that you did the challenge and submitted it! Have a wonderful day :)

    1