Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [sc-25878] Implement Checkbox Component in NameKit React #453

Merged
merged 27 commits into from
Dec 27, 2024
+213 −123
Merged

feat: [sc-25878] Implement Checkbox Component in NameKit React #453

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
c9733c2
feat: create Checkbox component in namekit-react
eduramme Oct 29, 2024
a9e4ea5
feat: update Checkbox style
eduramme Oct 29, 2024
2dbdc1f
feat: add the disabled variant style
eduramme Oct 29, 2024
2f91c7e
feat: create Checkbox Custom
eduramme Oct 30, 2024
f40de57
feat: disable hover effects when disabled
eduramme Oct 30, 2024
1a4637d
feat: remove hover effects when disabled
eduramme Oct 30, 2024
5023c12
feat: create Checkbox item with description
eduramme Nov 5, 2024
1ccd60f
refactor: Checkbox code
eduramme Nov 5, 2024
549f14e
test sotrybook
eduramme Nov 5, 2024
d06a0fe
feat: create label (children) logic
eduramme Nov 5, 2024
12dce64
feat: create EnabledUnchecked and EnabledChecked stories
eduramme Nov 5, 2024
9e68b88
feat: create label stories
eduramme Nov 5, 2024
521c11c
refactor checkbox
eduramme Nov 5, 2024
88f9e64
Merge branch 'main' into eduardoramme/sc-25878/checkbox-namekit-react
eduramme Nov 6, 2024
3fd741f
run changesets
eduramme Nov 6, 2024
808ee4e
fix storybook styles
eduramme Nov 6, 2024
bd470b8
remove unused variables
eduramme Nov 6, 2024
5bb25cb
fix label styling
eduramme Nov 6, 2024
a227968
fix label styling
eduramme Nov 6, 2024
d3524e8
improve CheckBox.stories
eduramme Nov 6, 2024
8237d32
remove "@types/node" from namekit-react
eduramme Nov 6, 2024
870fe0b
include own css reset input styles
eduramme Nov 6, 2024
8bed736
remove root README warning
eduramme Nov 6, 2024
c5d18fa
improve Checkbox stories
eduramme Nov 6, 2024
9d14070
apply inline-flex to Checkbox
eduramme Nov 6, 2024
0608340
remove description from the checkbox in SettingsModal
eduramme Nov 6, 2024
a36383a
Merge branch 'main' into eduardoramme/sc-25878/checkbox-namekit-react
eduramme Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/lovely-lemons-design.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@namehash/namekit-react": minor
---

Added new Checkbox component to the package. The Checkbox component allows rendering checkboxes with labels in a consistent style.
1 change: 0 additions & 1 deletion apps/nameguard.io/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
"sugar-high": "0.7.0"
},
"devDependencies": {
"@tailwindcss/forms": "0.5.9",
"@types/node": "22.7.4",
"@types/react": "18.3.11",
"@types/react-dom": "18.3.0",
Expand Down
5 changes: 1 addition & 4 deletions apps/nameguard.io/tailwind.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,7 @@ const config: Config = {
},
},
},
plugins: [
require("@tailwindcss/forms")({ strategy: "class" }),
require("tailwind-scrollbar-hide"),
],
plugins: [require("tailwind-scrollbar-hide")],
};

export default config;
119 changes: 119 additions & 0 deletions apps/storybook.namekit.io/stories/Namekit/Checkbox.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
import React from "react";
import { Meta, StoryObj } from "@storybook/react";
import { Checkbox } from "@namehash/namekit-react";

const meta: Meta<typeof Checkbox> = {
title: "UI/Checkbox",
component: Checkbox,
argTypes: {
disabled: { control: "boolean" },
checked: { control: "boolean" },
},
eduramme marked this conversation as resolved.
Show resolved Hide resolved
};

export default meta;

type Story = StoryObj<typeof Checkbox>;

export const Default: Story = {};

export const EnabledUnchecked: Story = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eduramme This story isn't working for me. I can't check the checkbox. Can you please investigate?

args: {
checked: false,
disabled: false,
},
};

export const EnabledChecked: Story = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eduramme This story isn't working for me. I can't uncheck the checkbox. Can you please investigate?

args: {
checked: true,
disabled: false,
},
};

export const Disabled: Story = {
args: {
disabled: true,
},
};

export const DisabledChecked: Story = {
args: {
checked: true,
disabled: true,
},
};

eduramme marked this conversation as resolved.
Show resolved Hide resolved
export const CustomStyling: Story = {
args: {
style: {
width: "24px",
height: "24px",
backgroundColor: "blue",
borderWidth: "2px",
borderColor: "red",
transition: "all 0.3s ease",
},
},
};

export const WithLabel: Story = {
eduramme marked this conversation as resolved.
Show resolved Hide resolved
args: {
children: "Accept Terms & Conditions",
},
};

export const DisabledWithLabel: Story = {
args: {
children: "Accept Terms & Conditions",
disabled: true,
},
};

export const WithCustomLabel: Story = {
args: {
children: <p style={{ color: "red" }}>Accept Terms & Conditions</p>,
},
};

export const WithDescriptiveText: Story = {
args: {
children: (
<>
<p>Accept Terms & Conditions</p>
<p
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

style={{
color: "gray",
fontWeight: "normal",
}}
>
I agree to the Terms of Service and Privacy Policy. By checking this
box, I acknowledge that I have read and understood all terms and
conditions.
</p>
</>
),
disabled: false,
},
};

export const DisabledWithDescriptiveText: Story = {
args: {
children: (
<>
<p>Accept Terms & Conditions</p>
<p
style={{
color: "gray",
fontWeight: "normal",
}}
>
I agree to the Terms of Service and Privacy Policy. By checking this
box, I acknowledge that I have read and understood all terms and
conditions.
</p>
</>
),
disabled: true,
},
};
167 changes: 62 additions & 105 deletions packages/nameguard-react/src/components/SettingsModal/SettingsModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React, { useState, Fragment } from "react";
import { Dialog, Transition } from "@headlessui/react";
import { XMarkIcon } from "@heroicons/react/24/solid";
import { Button, IconButton } from "@namehash/namekit-react";
import { Button, IconButton, Checkbox } from "@namehash/namekit-react";

import { useSettingsStore, type Settings } from "../../stores/settings";

Expand Down Expand Up @@ -71,118 +71,75 @@ export const SettingsModal = () => {
Adjust these settings based on your preferences.
</legend>
<div className="space-y-4">
<div className="relative flex items-start">
<div className="flex h-6 items-center">
<input
id="attempt-ens-normalization"
aria-describedby="attempt-ens-normalization-description"
name="attempt-ens-normalization"
type="checkbox"
className="form-checkbox ng-h-4 ng-w-4 ng-rounded ng-border-black ng-text-black !ring-black"
checked={localSettings.attemptEnsNormalization}
onChange={(e) =>
setLocalSettings((prev) => ({
...prev,
attemptEnsNormalization: e.target.checked,
}))
}
/>
</div>
<div className="ml-3 text-sm leading-6">
<label
htmlFor="attempt-ens-normalization"
className="font-medium text-gray-900"
>
Attempt normalization
</label>
<p
id="attempt-ens-normalization-description"
className="text-gray-500 text-sm leading-5"
>
Attempt ENS Normalization before inspecting search
queries. If normalization fails the raw search query
will be inspected instead.
</p>
</div>
</div>
<Checkbox
id="attempt-ens-normalization"
aria-describedby="attempt-ens-normalization-description"
name="attempt-ens-normalization"
checked={localSettings.attemptEnsNormalization}
onChange={(e) =>
setLocalSettings((prev) => ({
...prev,
attemptEnsNormalization: e.target.checked,
}))
}
>
<p>Attempt normalization</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

or something like that for the children?

Appreciate your advice 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 className="text-gray-500 text-sm leading-5 font-normal">
Attempt ENS Normalization before inspecting search
queries. If normalization fails the raw search query
will be inspected instead.
</p>
</Checkbox>

<div className="relative flex items-start">
<div className="flex h-6 items-center">
<input
id="assume-tld"
aria-describedby="assume-tld-description"
name="assume-tld"
type="checkbox"
className="form-checkbox ng-h-4 ng-w-4 ng-rounded ng-border-black ng-text-black !ring-black"
checked={localSettings.assumedTld === "eth"}
onChange={(e) =>
setLocalSettings((prev) => ({
...prev,
assumedTld: e.target.checked ? "eth" : null,
}))
}
/>
</div>
<div className="ml-3 text-sm leading-6">
<label
htmlFor="assume-tld"
className="font-medium text-gray-900"
>
Assume &quot;.eth&quot;
</label>
<p
id="assume-tld-description"
className="text-gray-500 text-sm leading-5"
>
Automatically adds “.eth” as an assumed top-level
name.
</p>
</div>
</div>
<Checkbox
id="assume-tld"
aria-describedby="assume-tld-description"
name="assume-tld"
checked={localSettings.assumedTld === "eth"}
onChange={(e) =>
setLocalSettings((prev) => ({
...prev,
assumedTld: e.target.checked ? "eth" : null,
}))
}
>
<p>Assume ".eth"</p>
<p className="text-gray-500 text-sm leading-5 font-normal">
Automatically adds “.eth” as an assumed top-level
name.
</p>
</Checkbox>

<div className="relative flex items-start">
<div className="flex h-6 items-center">
<input
id="trim-whitespace"
aria-describedby="trim-whitespace-description"
name="trim-whitespace"
type="checkbox"
className="form-checkbox ng-h-4 ng-w-4 ng-rounded ng-border-black ng-text-black !ring-black"
checked={localSettings.trimWhitespace}
onChange={(e) =>
setLocalSettings((prev) => ({
...prev,
trimWhitespace: e.target.checked,
}))
}
/>
</div>
<div className="ml-3 text-sm leading-6">
<label
htmlFor="trim-whitespace"
className="font-medium text-gray-900"
>
Trim whitespace
</label>
<p
id="trim-whitespace-description"
className="text-gray-500 text-sm leading-5"
>
Remove any leading or trailing whitespace characters
before performing inspection.
</p>
</div>
</div>
<Checkbox
id="trim-whitespace"
aria-describedby="trim-whitespace-description"
name="trim-whitespace"
checked={localSettings.trimWhitespace}
onChange={(e) =>
setLocalSettings((prev) => ({
...prev,
trimWhitespace: e.target.checked,
}))
}
>
<p>Trim whitespace</p>
<p className="text-gray-500 text-sm leading-5 font-normal">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CleanShot 2024-11-06 at 17 25 51

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 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way, we could ensure that text is alined with the label

Remove any leading or trailing whitespace characters
before performing inspection.
</p>
</Checkbox>
</div>
</fieldset>

<div className="flex items-center justify-end space-x-3 mt-8 pb-6">
<Button variant="secondary" onClick={closeModal}>
<Button
type="button"
eduramme marked this conversation as resolved.
Show resolved Hide resolved
variant="secondary"
onClick={closeModal}
>
Cancel
</Button>
<Button type="submit" onClick={closeModal}>
Save
</Button>
<Button type="submit">Save</Button>
</div>
</form>
</Dialog.Panel>
Expand Down
1 change: 1 addition & 0 deletions packages/namekit-react/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
"react": "18.3.1",
"react-dom": "18.3.1",
"tailwindcss": "3.4.13",
"@tailwindcss/forms": "0.5.9",
"tsup": "8.3.5",
"typescript": "5.6.2"
},
Expand Down
40 changes: 40 additions & 0 deletions packages/namekit-react/src/components/Checkbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import cc from "classcat";
import React, { InputHTMLAttributes } from "react";

export const Checkbox = ({
eduramme marked this conversation as resolved.
Show resolved Hide resolved
className,
id,
children,
disabled,
...props
}: InputHTMLAttributes<HTMLInputElement>) => {
return (
<label
className={cc([
"nk-relative nk-flex nk-items-start nk-flex-row",
disabled ? "nk-cursor-default" : "nk-cursor-pointer",
])}
>
<div className="nk-flex nk-h-6 nk-items-center">
<input
type="checkbox"
id={id}
disabled={disabled}
{...props}
className={cc([
"nk-form-checkbox disabled:nk-cursor-default nk-h-4 nk-w-4 nk-rounded nk-border-[1.25px] nk-border-gray-300 nk-text-black nk-cursor-pointer checked:nk-ring-0 focus:nk-ring-0 focus:nk-ring-transparent focus:nk-outline-none focus-visible:nk-border-gray-400 enabled:hover:nk-border-gray-400 disabled:nk-opacity-50 disabled:nk-bg-gray-100 disabled:checked:nk-bg-gray-300",
className,
])}
/>
</div>
<div
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eduramme Appreciate this perspective. I've been thinking differently, but will approved to unblock this goal for us 👍 Thanks

className={cc([
"nk-ml-3 nk-text-sm nk-leading-6 nk-text-gray-900 nk-font-medium",
{ "nk-opacity-50": disabled },
])}
>
{children}
</div>
</label>
);
};
1 change: 1 addition & 0 deletions packages/namekit-react/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ export { ENSTextArea } from "./components/ENSTextArea";
export { Input } from "./components/Input";
export { ENSInput } from "./components/ENSInput";
export { Link } from "./components/Link";
export { Checkbox } from "./components/Checkbox";
1 change: 1 addition & 0 deletions packages/namekit-react/tailwind.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const config: Config = {
},
},
},
plugins: [require("@tailwindcss/forms")({ strategy: "class" })],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions here:

  1. Given how you've moved this dependency into namekit-react, should we update this documentation in the README.md file in the root of our monorepo?

    namekit/README.md

    Lines 518 to 522 in 85c8370

    ## Tailwind Configuration
    When configuring Tailwind CSS for use with this project, please note the following:
    If you're using the `@tailwindcss/forms` plugin in your `tailwind.config.ts`, you may encounter some styling conflicts with our components. To resolve this, you should use the plugin with the class strategy applied. Here's an example of how to configure it: https://github.com/tailwindlabs/tailwindcss-forms?tab=readme-ov-file#using-only-global-styles-or-only-classes
    For example, exactly who needs to follow this instruction? Is it anyone building an app that adds namekit-react as a dependency? Or does this instruction only need to be followed if you import specific UI components from namekit-react? Etc.. Appreciate if you could write out a lot of details about this to make it more clear. I don't understand this currently.
  2. Can you help me understand more specifically why we need this plugin at all? I don't mind adding a plugin if it only impacts code directly in our own package. What I don't like is when we do things that cause unstandard complexity for everyone else who might use our package. For example, could we effectively replace the use of this plugin by just manually adding more classnames to the related form UI components?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This instruction only needs to be followed if the user imports our Input component from namekit-react. Other components, like our checkbox, are already compatible with the @tailwindcss/forms reset, so no additional configuration is needed for them. However, if the user imports the Input component and applies a global CSS reset (such as @tailwindcss/forms or similar), it may cause unintended styling issues with the Input component.

    To avoid these issues, users who import our Input component should follow the instructions to configure @tailwindcss/forms with the class strategy, or alternatively, we could add the reset class directly to the Input component to ensure it displays correctly in any setup.

  2. The @tailwindcss/forms plugin is basically a CSS reset focused on form elements. Browsers come with their own default styles for things like checkboxes, inputs, and textareas, which can make it tough to get a consistent look with Tailwind. This plugin wipes those defaults, making it easier to style form components the way we want.

    We added @tailwindcss/forms to namekit-react because our checkbox component already depends on that reset to look right. But our Input component doesn’t account for this reset yet. So, if someone using our package applies a global CSS reset on form elements, it can make our Input look weird.

    One way to handle this and get rid of the warning is to add the @tailwindcss/forms reset class directly to our Input component. That way, even if the user has a global CSS reset, our Input won’t be affected.

    This isn’t just a @tailwindcss/forms issue, it could happen with any CSS reset. We just noticed it specifically with @tailwindcss/forms because we were using it globally on Nameguard, which made the Input component look weird in forms there.

To wrap things up, this is a very specific issue, it only occurs if the user imports the Input component from namekit-react and has a CSS reset applied globally to form elements. Given how specific this is, I believe we have three options:

A: Remove the comment and let the user figure it out independently if that rare scenario happens.
B: Add the @tailwindcss/forms reset to our Input component.
C: Include a more generic warning, such as:
"This package includes custom styles that may be affected by other CSS resets or frameworks (e.g., Tailwind CSS forms). If you encounter unexpected styling issues, consider checking for conflicts with any global or opinionated CSS resets in your project."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what happens with our contact form, for example
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lightwalker-eth I’ve added a few reset classes to our Input component and removed the warning about Tailwind forms.

870fe0b

};

export default config;
Loading