-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
[RadioGroup] Create new RadioGroup
component
#487
Conversation
Netlify deploy preview |
packages/mui-base/src/RadioGroup/Indicator/RadioGroupIndicator.tsx
Outdated
Show resolved
Hide resolved
/** | ||
* The unique identifying value of the radio button in the group. | ||
*/ | ||
value: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to enforce strings here. Let's make it any
or unknown
as in Tabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A hidden <input>
is rendered for the whole RadioGroup root which requires the native DOM type:
{props.name && (
<input type="hidden" name={props.name} value={checkedItem ?? ''} required={required} />
)}
So we can allow string
or number
. I don't think anything else needs to be allowed anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to the Select, we can allow arbitrary values here and expose a prop to control how it's serialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What valid use cases are there for something other than a string or number?
a319d4a
to
87e9b02
Compare
00dee60
to
f27e6eb
Compare
9b9e040
to
e866d9d
Compare
@atomiks The docs demo is missing the "indicator", I think that's one (or perhaps the main?) point of confusion. There should be (whatever "should" means) a little dot inside the circle when it's selected. Like this https://helios.hashicorp.design/assets/components/form/radio-card/radio-card-states-2acdd2c7a5775a7479f7c497d3947804.png |
@colmtuite that's just one particular design though. The indicator can be anything, such as a plain filled-in white circle which is an outline when unchecked. |
@atomiks I don't see the |
import { useEnhancedEffect } from '../../utils/useEnhancedEffect'; | ||
import { useCompositeListContext } from './CompositeListContext'; | ||
|
||
export interface UseCompositeListItemParameters { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atomiks is Composite/CompositeItem intended to be a successor to the current useCompoundParent/Item?
At least for Slider I think this could be a completely replacement for it if we could pass arbitrary metadata into these parameters (for slider it's the id
of the associated input element)
BTW the only other component also using useCompound
is Tabs, which uses useCompound in a similar way that Accordion needs, I'll try using CompositeList/Item for Accordion first ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a replacement for both them & FloatingList
. It can be modified wherever necessary for different components
Closes #26