feat: [sc-25878] Implement Checkbox Component in NameKit React #453
Conversation
🦋 Changeset detectedLatest commit: a36383a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
eduramme
requested review from
djstrong,
Carbon225,
notrab,
lightwalker-eth,
FrancoAguzzi and
BanaSeba
as code owners
There was a problem hiding this comment.
@eduramme Thanks for improvements. Reviewed and shared a few suggestions 👍
| ])} | ||
| /> | ||
| </div> | ||
| <div |
There was a problem hiding this comment.
I'm thinking it's best that we not apply any classes to the children inside Checkbox. It seems better composability to me to just pass in {children} without any classes at all. This includes for the "disabled" case. If someone wants to apply special logic for disabled on children, they should manually implement that in their own child styles.
There was a problem hiding this comment.
@lightwalker-eth Any styles the user applies to the label will replace the default ones we set. Adding classes to the checkbox’s children can make things easier, especially if the user just passes a simple string (which is probably what most people will do).
Also, keep in mind that the label and checkbox are separate HTML elements. If the user wants, they can create a custom setup like this:
<Checkbox id="checkbox-id" />
<label htmlFor="checkbox-id" />
The main benefit of including the label inside the checkbox component is that it’s already styled, so the user doesn’t have to think much about it (it just works out of the box).
Makes sense?
There was a problem hiding this comment.
@eduramme Appreciate this perspective. I've been thinking differently, but will approved to unblock this goal for us 👍 Thanks
| })) | ||
| } | ||
| > | ||
| <p>Attempt normalization</p> |
There was a problem hiding this comment.
I'm not sure what the best practice is here. It's interesting for me how children isn't required to be a single node? For example, here I see children defined as two
nodes that are siblings. Can you please advise if this is ok, or if we should change anything so that children is always a single node? For example, force someone to wrap two sibling
nodes in a parent
children?
Appreciate your advice 👍
There was a problem hiding this comment.
The children prop is not restricted to a single node. It can be a single node, multiple nodes, or even a string. This flexibility allows for more dynamic and composable UI structures. Having two node elements as children of the Checkbox component is perfectly fine.
| } | ||
| > | ||
| <p>Trim whitespace</p> | ||
| <p className="text-gray-500 text-sm leading-5 font-normal"> |
There was a problem hiding this comment.
I don't think it's best that we would also make the descriptive text here as part of the hover / click area for the checkbox. Only the label for the textbox "Trim whitespace" should be part of the hover / click area. "Remove any leading or trailing whitespace characters before performing inspection." should not be part of the hover / click area and shouldn't be a child of <Checkbox>.
What do you think? Appreciate your advice 👍
There was a problem hiding this comment.
Sounds good. We will have that empty space, that will be covered by some padding or margin. Do you think it's a bad idea to create a "description" prop for the Checkbox component?
There was a problem hiding this comment.
This way, we could ensure that text is alined with the label
| ])} | ||
| /> | ||
| </div> | ||
| <div |
There was a problem hiding this comment.
@eduramme Appreciate this perspective. I've been thinking differently, but will approved to unblock this goal for us 👍 Thanks
| }, | ||
| }; | ||
|
|
||
| export const EnabledUnchecked: Story = { |
There was a problem hiding this comment.
@eduramme This story isn't working for me. I can't check the checkbox. Can you please investigate?
| }, | ||
| }; | ||
|
|
||
| export const EnabledChecked: Story = { |
There was a problem hiding this comment.
@eduramme This story isn't working for me. I can't uncheck the checkbox. Can you please investigate?
|
|
||
| export const WithoutLabel: Story = {}; | ||
|
|
||
| export const WithAdvancedChildren: Story = { |
There was a problem hiding this comment.
@eduramme Hey can you please fix the vertical alignment of the checkbox in this story?
The checkbox should appear in the same line as "Accept Terms & Conditions"
| children: ( | ||
| <> | ||
| <p>Accept Terms & Conditions</p> | ||
| <p |
There was a problem hiding this comment.
@eduramme Hey I don't think we should make the "advanced children" part of the area that toggles the checkbox in this story. We should have the UX in this story work similar to how everything works in the NameGuard settings modal.
lightwalker-eth
deleted the
eduardoramme/sc-25878/checkbox-namekit-react
branch
Story details: https://app.shortcut.com/ps-web3/story/25878