Checkbox Component with Tailwind #654
Conversation
|
Hey @LoTerence , thanks for reviewing my PR, which I'm not very confident about. My understanding of how a checkbox is focused seems different from the SASS styles in the existing Checkbox component. I see a transition and duration in the SASS code, but Tailwind's peer-focus seems to apply the styles as long as the input is focused. What's correct? |
|
Hey Bitian, I understand your confusion. Its a pretty complex component now that I'm looking at it.
Its not that one is "correct" and the other is "incorrect". They are simply two different approaches of applying the highlight effect style. The difference is in the behavior. I would like to imitate the behavior of the previous component as much as possible. So the style shouldnt be applied only when the input is focused. Before, it looks like the style was applied when the checkbox is hovered over. We should try to implement it this way as well. Let me do the Checkbox component. I am going to totally redo it, so its going to be pretty different from Avas version. I will make a new branch off this branch to work off of, and close this PR. |
|
Wait a second, I just saw the figma, there is no hover state in Rishi's version of the Checkbox. Is this what you were asking about? Because that changes the behavior of the checkbox entirely. Could you follow up with Rishi and clarify with him about how the Checkbox is supposed to behave? Your version in tailwind-demo might actually be more correct. Sorry about the confusion. |
|
Tagging issue #583 |
|
@LoTerence Thanks for tagging the issue! Regarding the step "once the component is fully tested and verified working, remove the .scss file associated with that component and refactor away any old code", should I do this now or should that be done after this PR has been reviewed and merged in? |
|
Hey @bzzz-coding If you could do it now to knock out two birds with one stone, that would be good. If you want to open a new PR for that part, we can do that too. Check out Benny's PR #663 for an example of fully converting a component from scss to tailwind - it removes the scss and refactors away old code too. |
| @@ -16,6 +15,7 @@ import { | |||
| IconDropdownDown, | |||
| } from "assets/images/images"; | |||
| import { combineClasses } from "components/Utility/utils"; | |||
| import Checkbox from "tw-components/Checkbox"; | |||
There was a problem hiding this comment.
Lets remove all Checkbox code from Demo.tsx entirely, since the new Checkbox will be in /demo-tailwind anyway. I don't think we need to see it in two places.
There was a problem hiding this comment.
Lets also bring the checkboxOnChange function over to DemoTailwind.tsx so we can test that onChange works.
I tested it just now and can confirm it works ✅
|
|
||
| // Type declaration for props | ||
| interface CheckboxProps { | ||
| additionalClass?: string; |
There was a problem hiding this comment.
| additionalClass?: string; | |
| className?: string; |
Lets change this prop name to className, so devs can style the <Checkbox /> component using the className property instead of having to use another name like additionalClass.
Check out CircleCard.tsx to see an example of how to use props.className with clsx()
| className={clsx( | ||
| "absolute h-10 w-10 rounded-full opacity-0", | ||
| "peer-focus:bg-blue-dark peer-focus:opacity-[.16] peer-active:bg-blue-dark peer-active:opacity-[.32]", | ||
| { hidden: disabled }, // no effect when disabled |
There was a problem hiding this comment.
| { hidden: disabled }, // no effect when disabled |
We don't need this line since HTML elements can't be focused when they are disabled: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Attributes/disabled#constraint_validation
I like how you used tailwind peer-focus to implement the focus effect though, great job!!
| htmlFor={checkboxId} | ||
| className={clsx( | ||
| "relative flex cursor-pointer select-none items-center", | ||
| { "opacity-50 cursor-not-allowed": disabled }, |
There was a problem hiding this comment.
| { "opacity-50 cursor-not-allowed": disabled }, | |
| disabled ? "cursor-not-allowed opacity-50" : "cursor-pointer", |
I tested the original code -- your code is correct but when I hover over a disabled element, it still has cursor-pointer.
Found a bug where for some reason CSS is not keeping the order of the classnames, and is prioritizing cursor-pointer over cursor-not-allowed. I believe it has something to do with how tailwind is set up to use PostCSS modules: JedWatson/classnames#112 (comment)
I will make a new issue to address this. For the time being let's use a ternary operator to set the cursor styling.
| )} | ||
| </span> | ||
| <span | ||
| className={clsx({ "sr-only": labelHidden, "ml-1": !labelHidden })} |
There was a problem hiding this comment.
| className={clsx({ "sr-only": labelHidden, "ml-1": !labelHidden })} | |
| className={ labelHidden ? "sr-only" : "ml-1" } |
I think using ternary operator here is cleaner.
Thanks for making sure screen reader styles are applied! This is really important for input components.
| @@ -7,7 +7,7 @@ import "regenerator-runtime/runtime"; | |||
| import { config } from "react-transition-group"; | |||
|
|
|||
| // Internal imports | |||
| import { Checkbox } from "components/components"; | |||
| import Checkbox from "tw-components/Checkbox"; | |||
There was a problem hiding this comment.
I'm going to create an issue about how these tests are outdated and might break. We need a full audit of the frontend unit tests.
| ); | ||
| } | ||
|
|
||
| export default Checkbox; |
There was a problem hiding this comment.
One last thing: Lets add an export of Checkbox to tw-components/index.tsx, to keep all the exports in one place.
Changes
Screenshots, if applicable
Screenshot