Skip to content
OverviewSolutions
10
Comments
16
SutonToch
@SutonToch

All comments

  • SutonToch•340
    @SutonTochPosted about 1 year ago

    Congrats on completing the challenge!

    Your solution looks great!

    One minor tweak on mobile: your .main-container is a little bit too big. I don't really know why height: auto is doing this.. but with height: fit-content it looks much better.

    In general, you can check the responsiveness in your browser with Strg+Shift+M (responsive mode). There you can also use F12 (Dev-Tools) to debug your website. With that you would notice, that some of your CSS-rules don't really do anything, e.g.: body {padding: 0} (maybe you meant margin? Because every body has a base margin of 8px), .main-container {flex-direction: row; margin-top: 0em} (both are default values). Removing unnecessary code (and checking for it) is good, because you'll have less code to read and maintain, which makes everything else easier.

    If you want to learn more about responsiveness, I can personally recommend "Conquering Responsive Layouts" by Kevin Powell. It's a free course, and it helped me tremendously when I started out, but you're a little further in your Frontend Journey, than I was at the time, so your mileage may vary. Maybe there is still something that helps :)

    I could give you a few more suggestions for "more optimized/better coding", but I'm not quite sure if it would actually help you. Coding is an iterative process, and you'll simply improve over time, for example by noticing small annoyances (e.g. having to repeat code) and then trying to solve them. If you really want, though, here are three things you could look into:

    • Semantic HTML (especially important for Screen Reader (Accessibility) and Search Engine Optimization (SEO))
    • other units than px (sometimes more useful), e.g.: rem, em, vw, vh
    • CSS-Variables for less repetition once they're set up

    Finally, please take your time. Only because I suggest something, doesn't me you'll immediately have to look into it. Learning to program requires a lot of time and practice, and there is real risk of burning yourself out.

    I'm looking forward to your next solution :)

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

    Congrats on completing the challenge!

    • You could add cursor: pointer to your .btn-continue so that it is a little clearer, that you can click it
    • Your .summary-item would probably benefit from a background-color, similar to the specifications, because currently they kinda meld into the background. For that, you'll probably have to also add some padding, and reduce the margin-bottoms. Another way would be to add a light background-color to the body, so that the white background of the .summary-container stands out. Yet another way is to add a box-shadow to your entire .container.
    • You should also switch to mobile a little earlier than 375px. I think some of the smallest phones have 360px in width, but there are many with more, and those currently get the desktop-design. Maybe a breakpoint for 600px and below would be good, but there are many approaches to these breakpoints, and I'd recommend reading up on that if you have the time.

    All in all, your design looks great! When I first started out, I quite struggled with this challenge.. especially centering the circle and it's content. Also, your solution to the .score in the .summary-container is smooth. As a general hint, maybe use a little bit more padding, especially on containers. With padding on .container, you'll need less margin on .result-text, .description and .summary-text. And if you want to change it, you'll only have to change it in one place.

    If you want to further improve, I'd recommend taking a look at CSS-Variables. And finally, if you have any more questions, feel free to throw them my way, I'm always happy to help.

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

    1
  • devusexu•130
    @devusexuSubmitted about 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 about 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 about 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 about 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 about 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 about 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 about 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 about 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 about 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 about 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 about 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 about 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 about 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 about 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 about 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