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

All comments

  • Namrata Sinha•530
    @nmrtsnhSubmitted 12 months ago

    Hello everyone,

    I am Namrata and this is my solution for the Newsletter sign-up form with success message challenge.

    If you have any suggestions to improve my code, please feel free to share!

    Thank you 🙏

    Newsletter sign-up form with success message

    2
    Josué Zalazar•80
    @josuez2006Posted 12 months ago

    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
    0
  • lasse-cs•50
    @lasse-csSubmitted about 1 year ago

    I did not know how the hover labels should be done and positioned.

    I used absolute positioning, but I am not sure how best to center them above the bar. If there were more digits, I think they would not be centered now.

    Expenses Chart

    1
    Josué Zalazar•80
    @josuez2006Posted 12 months ago

    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.

    0
  • Josué Zalazar•80
    @josuez2006Posted 12 months ago

    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.

    1
  • Rodrigo•480
    @RodrigoHLCSubmitted 12 months ago

    Spent about 2:15 h on the layout and 1:15 on the javascript. I wrote it so that no matter how many days are added to the API, this will always use the last 7 days (though I shouldn't brag too much as that can be done in just one line of code), and the height of the bars will be set in relation to the highest value of the week, which will always have a height of 130px📊 The user can click on multiple bars to freeze the tooltips. I'm very, very happy with this one as I churned it out in about 3 hours and 40 minutes, give or take😃😃 got to learn a couple of new things and came up fairly quickly with a workaround for an issue I was having in regards to the use of CSS selectors.

    15th Task📊🔥 Very Happy with this one. Quick completion time, I think

    1
    Josué Zalazar•80
    @josuez2006Posted 12 months ago

    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.

    0
  • Josué Zalazar•80
    @josuez2006Posted 12 months ago

    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?

    0
  • Josué Zalazar•80
    @josuez2006Posted 12 months ago

    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.

    1