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.

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

Adding Radio Input component #27

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ivyalin2015
Copy link

@ivyalin2015 ivyalin2015 commented Dec 15, 2024

Adding Radio Input component

♻️ Current situation & Problem

Add Radio input component

⚙️ Release Notes

  • Allows users to select one option from a dropdown list with as many items as desired
  • Uses standardized coloring

📚 Documentation

Please ensure that you properly document any additions in conformance to Spezi Documentation Guide.

This RadioInput component provides a template to create a multiple choice question.
image

✅ Testing

Please ensure that the PR meets the testing requirements set by CodeCov and that new functionality is appropriately tested.
My tests mainly focus on rendering but do not include confirmation that different items can be selected.

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@ivyalin2015 ivyalin2015 changed the title Audrey/checkbox Adding Radio Input component Dec 15, 2024
@arkadiuszbachorski arkadiuszbachorski self-requested a review January 2, 2025 18:02
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/components/Checkbox/Checkbox.stories.tsx Outdated Show resolved Hide resolved
src/RadioInput/RadioInput.stories.tsx Outdated Show resolved Hide resolved
defaultValue="default"
aria-label="View density"
>
<div className="flex items-center space-x-2">
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit too complex to be repeated each time. I'd love to have SideLabel component that renders correct input + label element for both Checkbox and Radio.

I imagine such usage:

<SideLabel label="Second choice">
  <Radio value="2" />
</SideLabel>

If <label> contains input, there is no need for htmlFor and id, it's implicit!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a note, we already have SideLabel component.

export default meta

export const Default = () => (
<RadioInput
Copy link
Collaborator

@arkadiuszbachorski arkadiuszbachorski Jan 6, 2025

Choose a reason for hiding this comment

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

We should have a component that renders group of radios already styled. I.e. we want to make sure common need of rendering multiple radios does not need any additional styling. Something like this:

<RadioGroup>
  <SideLabel label="One">
   <Radio id="1" value="1">
  </SideLabel>
</RadioGroup>

src/RadioInput/RadioInput.tsx Outdated Show resolved Hide resolved
<RadioPrimitive.Item
ref={ref}
className={cn(
'relative flex h-6 w-6 items-center justify-center rounded-full border-2 border-border focus:ring-2 focus:ring-ring focus:ring-offset-1',
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have flex-center class that is a shorthand for flex items-center justify-center. Extremely often used.

h-6 w-6 can be represented as size-6. It's worth to indicate that both dimensions are meant to be the same.

We have focus-ring class for focus properties. It should do the trick as well here.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm... I was having an issue of the focus-ring disappearing when I used focus-ring, like this:

image

Just to make sure, should the tailwind code look something like this?
className={cn( 'relative flex-center size-6 rounded-full border-2 border-secondary focus-ring', className, )}

Copy link
Collaborator

@arkadiuszbachorski arkadiuszbachorski Jan 15, 2025

Choose a reason for hiding this comment

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

This would be an ideal implementation of Radio with indicator and styles.

export const Radio = forwardRef<
  ElementRef<typeof RadioPrimitive.Item>,
  ComponentPropsWithoutRef<typeof RadioPrimitive.Item>
>(({ className, ...props }, ref) => (
  <RadioPrimitive.Item
    ref={ref}
    className={cn(
      'focus-ring flex-center size-4 shrink-0 rounded-full border border-input disabled:cursor-not-allowed disabled:opacity-50',
      className,
    )}
    {...props}
  >
    <RadioPrimitive.Indicator className="flex-center size-2.5 rounded-full bg-primary" />
  </RadioPrimitive.Item>
))
Radio.displayName = RadioPrimitive.Item.displayName

Main aspects:

  • focus is functional due to focus-ring
  • size is exactly the same as Checkbox, but it's round, keeping the UI consistent and easy to match up together
  • disabled state is handled

Comment on lines 32 to 36
{label && (
<label htmlFor={props.id} className="popover-foreground">
{label}
</label>
)}
Copy link
Collaborator

@arkadiuszbachorski arkadiuszbachorski Jan 6, 2025

Choose a reason for hiding this comment

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

Let's keep label separate from the Radio item itself. By relying on composition through separate components, it makes it easier to build reusable components. Labels can quite often require customization, so it's better just to keep it as separate component, using elements like SideLabel (mentioned in other comment)

))
RadioItem.displayName = RadioPrimitive.Item.displayName

export const RadioIndicator = forwardRef<
Copy link
Collaborator

@arkadiuszbachorski arkadiuszbachorski Jan 6, 2025

Choose a reason for hiding this comment

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

Every RadioItem is always going to have Indicator. Could we just put the indicator inside the RadioItem? If someone needs something custom on this matter, they usually are going to opt-out from this component anyway.

What I value here is simplicity of usage. With RadioIndicator baked into the Radio component, we'd just have simpler API usage.

@arkadiuszbachorski
Copy link
Collaborator

@ivyalin2015 Thank you for working on this! I left couple comments, could you please address them?

Copy link
Collaborator

@arkadiuszbachorski arkadiuszbachorski left a comment

Choose a reason for hiding this comment

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

We also need index.tsx file in the Radio directory that exports Radio components. This is necessary, because we want components/Radio to be a module itself.

Furthermore, we need Checkbox to be exported from src/index.tsx (to make it accessible from @stanfordspezi/spezi-web-design-system module) and from package.json (to make it accessible as @stanfordspezi/spezi-web-design-system/components/Radio).

Referencing this PR should help: https://github.com/StanfordSpezi/spezi-web-design-system/pull/36/files

@@ -206,11 +206,13 @@
"@radix-ui/react-checkbox": "^1.1.2",
"@radix-ui/react-dialog": "^1.1.2",
"@radix-ui/react-dropdown-menu": "^2.1.2",
"@radix-ui/react-icons": "^1.3.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need these dependencies.

)}
{...props}
/>
{label && <SideLabel className="popover-foreground">{label}</SideLabel>}
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I clarified on Slack, label should be removed from this component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants