Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Request path contains unescaped characters
Request path contains unescaped characters
Not Found
Not Found
Not Found

Submitted

Mobile first page with CSS, JS, BEM, Semantic HTML5

Cássia 90

@casbern

Desktop design screenshot for the Pricing component with toggle coding challenge

This is a solution for...

  • HTML
  • CSS
  • JS
2junior
View challenge

Design comparison


SolutionDesign

Solution retrospective


Hi guys!

Is there anything I can improve on my HTML, CSS e JS? How to make it shorter just to not repeat myself so much in my code? I tried to make it as concise as I could. I just do not know if I am in the right direction.

Thank you very much in advance.

Community feedback

@atinybeardedman

Posted

Hi there, I think there's a few things you can do to improve this one. I wouldn't say the goal is always conciseness, but rather readability. Concise is great, but being able to read and understand code is better. So here are some suggestions/general feedback:

  • In javascript try to avoid comparing things directly to boolean values in a condition. Instead just use the boolean directly. So in your case you can just check if(checkedValue.checked){...}. Along those lines I would consider a clearer variable name. So in your case the "checked" state means they want monthly amounts. So I would name it something like isMonthlyCheckBox or something like that so it's super clear what the logical value is associated with. Also in here you have a minor bug with the array indexes. You are making the pro plan cheaper than the basic plan in the js.
  • I think your css BEM style looks really good here. You have clearly distinct namespaces for the different sections they are easy to read and understand.
  • Your html markup looks good. These buttons don't do anything so it's a toss up if they should be buttons or a tags, but you have aria role button on the a tag which might actually be a bit confusing to screen readers since they already pick up a tags on their own. I might just stick with an a tag without the role button, but I'm not an accessibility expert.
  • You have a minor reflow issue when you hover the middle button on desktop. This is because the border doesn't exist on the non-hovered state, so the 2px that is added (1 for top and 1 for bottom) is enough to visibly shift the layout of the card. Not a big deal, but easily fixed by adding a 1 px white border to all buttons so that it's always there are really you are just changing the color on hover. In terms of these color changes, I'd highly recommend adding a css transition on the btn class for color, border-color, and background. It's subtle but it brings up the percieved user experience a bit.

Overall this looks great, nice work!

Marked as helpful

3

Cássia 90

@casbern

Posted

@atinybeardedman Hi Sean! Hope you are fine! I want to thank you very much for looking through my code and for giving me such valuable feedback. I really really appreciated it.

You were very detailed and that is all that I wanted and needed, cause I really want to improve. You even saw things that I did not and I looked a lot my code.

I took you advice and changed my code. I will also exercise now how to use the transition, because for some reason I do not find it very easy...

But I learned a few things that I did not know before with your feedback which is wonderful.

So thanks again!

0

Please log in to post a comment

Log in with GitHub
Discord logo

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord