Skip to content
OverviewSolutions
26
Comments
29

Jax Teller

@piushbhandari670 points

Yo! Wannabe concept artist/author

I’m currently learning...

backend stuff like node js and mongodb

Latest solutions

View more solutions

Latest comments

  • Jax Teller•670
    @piushbhandari

    i had a rough look at your JS and styles.

    make sure to:

    1. display:none; to your .accordion-text element to hide it when the accordion is not opened.
    2. in your JS, the class that's being added to the accordion button isn't doing anything. when checking if the element has .title__click, add .title__click to the button AND to the .accordion-text. in your css make sure to have this .title__click{display: block;} this is so that when the class gets added to both elements you'll be showing the .accordion-text element too. then when you click again you can use JS to remove .title__click

    BTW:

    • you can remove the input:checkbox and label. convert the label to a button instead, so <label for="section1" class="accordion__label title__click" id="title"> will be <button type="button" class="accordion__label title__click" id="title">
    • instead of having the svg +/- icons as an :after element, just put them into their own <img/> element instead inside the <button> element when you convert it from your label. use css to hide the - icon. when you attach the .title__click class to your button element use css to show the - icon and hide the + icon. then do the opposite when the .title__click class gets removed. right now when i click on your accordion buttons the + disappears and no - icon.
    • for you background image pattern, you can leave the alt attribute as alt="" since this is for decorative purposes only. i would also set the image to be width: 100%; so that when you zoom out your layout doesn't look cropped out

    hope this helps

    Marked as helpful
  • Jax Teller•670
    @piushbhandari
    • use .questions:last-child{} to remove the border + bottom margin/padding from the last question
    • any reason why <div class="question"> isn't a button element? right now i can't use your component with my keyboard. using semantic elements is always a good thing. there are more accessible things you can do but replacing it with a button element is a good start
    • assign classes instead of setting inline styles. i.e add active to the answer and give it display:flex;
    • if this <div class="title"> is being used as your main title replace the the div with <h1>
    Marked as helpful
  • Jax Teller•670
    @piushbhandari

    nice work.

    some suggestions:

    • i would make these <h2 class="text-day" style="color: rgb(113, 111, 111); transition: all 0.2s ease 0s;">Day</h2> into a label and hook it up to the number inputs. like so: <label for="day" class="text-day" style="color: rgb(113, 111, 111); transition: all 0.2s ease 0s;">Day</label> then add in name/id attributes that have the same value as the for attribute to the input. this way when you click on the label it auto focuses on the input. this is good for accessibility

    • your .text-day element has an inline style. is there a reason why it's not in a separate stylesheet?

    • it's best practice + good for accessibility if you add type="button" on your button elements

    • you can set the alt tag from the icon inside the button to be empty "" as it's useless information for screen reader users. when you have icons, having alt="" is good enough as it's being used as a decorative image

    • im not sure if there's a tablet design, but when you start to scale the design, the component starts hugging the edges of the screen. i would add side paddings ie. padding-left: 24px; padding-right: 24px; on the body element for example.

    • may need a 2nd opinion on this but those years/months/days text can just be regular <p> tags instead of <h2>s

  • Jax Teller•670
    @piushbhandari

    form validation needs to be stronger. i was able to get to the next screen with this: ggggsdff@com (no '.' after the @)

    by the way, i wouldn't use cursor: pointer; for the input and instead use the default style on browsers

    Marked as helpful
  • Jax Teller•670
    @piushbhandari

    • toggling class to show/hide content via javascript is a good approach and you'll see this as being best practice.

    • BTW, when i zoom out on your page, your layout gets messed up. i would change your styles on your body element to background-repeat: repeat-x; background-size: contain;. FYI, i would have the background image as a separate element

    • i would change <div class="question"> to <button type="button" class="question">. it's good to use semantic markup + as it stands right now your component is not accessible via keyboard and that's a no no. likely will need to update styles

    Marked as helpful
  • Jax Teller•670
    @piushbhandari
    • i would avoid using cursor: pointer; for inputs and instead use the browser default
    • you have a label but it's not hooked up to your email input. since your for attribute on your label is 'email' i would add attributes id="email" name="email". this is for accessibility. when you click on a label, it should then focus on the input it's associated to
    • you have a second label that i think you can just use the <p> element instead
    • right now, after entering a valid email, when i tried to submit your page leads to a 404 page. it looks like you're submitting the form with no url. i would either remove the outer form or use javascript to prevent the form submitting and instead you controlling what it does via code.

    let me know if you need help with the last one. essentially what i did in my markup was attach this onsubmit="return false;" to my form element which blocks the form from doing its native action

    Marked as helpful
View more comments