Skip to content
OverviewSolutions
4
Comments
6
Josué Zalazar
@josuez2006

All comments

  • Josué Zalazar•80
    @josuez2006

    Hey there, you did a fantastic work! 😄

    Consider these fixes to make it even better

    1 - Use data attributes to display the form or the thank-you modal

    <form class='sign-up' data-hidden=false>...</ form>
<section class='thank-you-modal' data-hidden=true>...</section>

    [data-hidden=false] {
     display: none;
}

[data-hidden=true] {
      display: flex;
}

    signUpForm.setAttribute('data-hidden', 'false');
thankYouForm.setAttribute('data-hidden', 'true');

    2 - Use the picture tag to change the image depending the screen size

    <picture>
    <source src="./assets/images/illustration-sign-up-desktop.svg" media="(min-width: 600px) />
     <img class="modal-img" src="./assets/images/illustration-sign-up-mobile.svg" alt="Form Illustration"/>
</picture>

    3 - Look at your span tags. Most of them are paragraphs and should be a p tag.

    <p class="description">
   Join 60,000+ product managers receiving monthly updates on:
</p>

    If you find this useful, please mark it as helpful

    Have an amazing day 😁

    Marked as helpful
  • Josué Zalazar•80
    @josuez2006

    Those labels are usually called tooltips and it's so simple to center them.

    First, you need the container's tooltip to have the property of position relative. So then, when you apply position absolute to the tooltip, it'll be relative to its container.

    Once that's done, you have to set the property left to 50% on the tooltip. In this way, it will start in the middle of the container (but hold on, it hasn't been centered yet).

    Lastly, on the tooltip, write 'translate: -50%' to move the tooltip to the left and center it.

  • Josué Zalazar•80
    @josuez2006

    Great solution though! It works. I didn't know that I could use a data attribute on a tag and use it for the content of the after pseudo-element. With that idea, I could build cleaner js code.

  • Josué Zalazar•80
    @josuez2006

    Fantastic work! I learned a lot by reading your code (it has good comments). In my solution, I didn't think about using the document.querySelectorAll and it made it more complicated than it could have been.

    To be honest, you could improve it by using const variables instead of let as they don't change, but it's not so bad. I love how you used the map and forEach functions to reduce the amount of code as well as the consideration for touch actions on mobile devices.

    I didn't expect the starting animation and it's amazing.

    Overall, the only issue that I find in your code as well as mine is the responsiveness of the chart boxes because they're fixed to a certain height.

  • Josué Zalazar•80
    @josuez2006

    In my opinion, using Tailwind to style this simple project makes it hard to read and understand what's happening. Would you be kind enough to explain to me why you use it?

  • Josué Zalazar•80
    @josuez2006

    Congratulations, you have built a responsive site. It appears that you have a passion for learning new things. Because of that, I'd like to give you some best practices that you could use to improve your skills.

    First of all, although it's a small project, you should separate the HTML and CSS into different files to have better readability.

    Secondly, HTML tags have meaning and you have to use them wisely. The div tag has no purpose in your code and the h3 tag has no sense since it's appropriate to use a h1 (it's the unique and more important heading on the site).

    Last but not least, as your project grows, you going to probably have more paragraphs with other styles, so it's a great practice to always use classes instead of referring to the HTML tag.

    I hope it helps you.