Skip to content
OverviewSolutions
15
Comments
74

Doston Nabotov

@dostonnabotov950 points

Dream it, Design it and Build it!

I’m currently learning...

React, TypeScript, Data Structures and Algorithms

Latest solutions

View more solutions

Latest comments

  • Doston Nabotov•950
    @dostonnabotov

    Hey, there! 👋

    Congrats on your first junior challenge solution. Here's some suggestions I would like to point out:

    • The cards didn't load on the page. I think it has to do with your JavaScript fetching code.
    • Opt for better naming convention for your classes. Since div-small, div-violet doesn't carry too much meaning, go with something like card, tracking-item and etc.
    • Also, I would avoid spamming <br> elements to give space between other elements. You can read more about its purpose on MDN Docs
    • CSS custom properties woud really help optimize your CSS code.
    • As the last suggestion, I would recommend reading and following the Frontend Mentor guidelines. Create a README.md file and explain what, how and why you did. Good for you and other fellow programmers.

    Hope it helps! Good luck on your coding journey!

    Marked as helpful
  • Doston Nabotov•950
    @dostonnabotov

    Hey, there!

    Congrats on completing the challenge.

    Your solution looks great, and I don't have much suggestions on your code.

    As of your question, visit CSS guidelines to learn more about CSS and how to write better.

    Good luck on your coding journey!

  • Doston Nabotov•950
    @dostonnabotov

    Hey, there!

    Congrats on completing the challenge.

    I really liked your approach on semantic markup and good CSS code.

    Here are some suggestions I would like to make:

    • As of your question, do not heavily rely on viewport units. Because if the user tries to zoom in or zoom out, it really gets out of control. Instead try switching it something like this:
    .profile {
  width: calc(100% - 2rem);
  max-width: 21.875rem;
  margin-inline: auto;
}

    Or if you want one line:

    .profile {
  width: min(100% - 2rem, 21.875rem);
  margin-inline: auto;
}

    100% - 2rem indicates that it can stretch to 100% full width, but with 1rem extra spacing on each side. I used margin-inline: auto just in case to horizontally center the element.

    • Speaking of fixed height on your image, I liked the descriptive comment explanation. But, here's the better way you might wanna consider:
    .profile__decoration {
  background-image: url("images/bg-pattern-card.svg");
  width: 100%;
  aspect-ratio: 16 / 9;
}

    Basically, you rely on your dynamically changing width to determine the height of your image. Find out what ratio it uses and apply it.

    Good luck on your coding journey!

    Marked as helpful
  • Doston Nabotov•950
    @dostonnabotov

    Hey, there!

    Congrats on completing the challenge.

    Here are some suggestions I would like to make:

    • Move your styles to separate style.css file.
    • Use Prettier. I see that you don't care about formatting and how your code looks. So, let Prettier (VS Code extension) handle it for you
    • Add some spacing on top and bottom of your page for small devices. Something like, padding-block: 2rem; on body element should do it.
    • Attribution is overflowing on top of other elements on small devices. Either remove it or style it better.
    • Read the README.md and README-template.md files if you want other developers check and read your code.

    Good luck on your coding journey!

    Marked as helpful
  • Doston Nabotov•950
    @dostonnabotov

    Hey, there!

    Congrats on completing the challenge.

    Here are some suggestions I would like to make:

    HTML:

    • Try using radio checkboxes <input type="radio"> instead of regular buttons. Because you only have to select one rating, it will make your life much easier. Also, less JavaScript code.

    CSS:

    • I think you're missing * selector in your first few lines in your code.
    • I wouldn't recommend setting transition on every element. Use transition property where you need it.
    *,
*::after,
*::before {
  box-sizing: border-box;
}

    JavaScript:

    • If you've switched to radio checkboxes by this time, you can safely remove your code where you toggle the state of buttons.

    Other than these, everything looks good to me.

    Good luck on your coding journey!

    Marked as helpful
  • Doston Nabotov•950
    @dostonnabotov

    Hi, there! 👋

    Congrats on completing the challenge!

    Really liked your solution. But, here are some suggestions I have:

    • I found a bug with your mobile navigation toggle. When using it on my phone, I noticed that icon and navigation state is switched. In other words, when I open nav menu, icon turns into "menu" instead of "x".

    Possible solution: I think it's causing by your JavaScript code, where you've specified the isIconEnabled to true. I think, by default it should be set to false, because nav menu will be closed when you first load the page.

    • (In my opinion) You have way too many media queries in your CSS. 3 and 4 queries are way too much to handle as if you are trying to micro managing your website.

    Possible solution: If you're having issues with big spacing on larger devices and small spacing on small devices, try better learning the clamp(). See how you unleash the power, by not just applying to font sizes, but also in your spacing and other several places.

    Good luck on your coding journey.

    Marked as helpful
View more comments