Skip to content

[RadioGroup] Create new RadioGroup component#487

Merged
atomiks merged 47 commits into
mui:masterfrom
atomiks:feat/RadioGroup
Sep 4, 2024
Merged

[RadioGroup] Create new RadioGroup component#487
atomiks merged 47 commits into
mui:masterfrom
atomiks:feat/RadioGroup

Conversation

@atomiks

@atomiks atomiks commented Jul 24, 2024

Copy link
Copy Markdown
Contributor

Closes #26

@atomiks
atomiks marked this pull request as draft July 24, 2024 05:51
@mui-bot

mui-bot commented Jul 24, 2024

Copy link
Copy Markdown

@atomiks
atomiks marked this pull request as ready for review July 25, 2024 06:55
@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jul 25, 2024
Comment thread packages/mui-base/src/RadioGroup/Indicator/RadioGroupIndicator.tsx
/**
* The unique identifying value of the radio button in the group.
*/
value: string;

Copy link
Copy Markdown
Member

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.

@atomiks atomiks Aug 7, 2024

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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?

@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Aug 7, 2024
@colmtuite

Copy link
Copy Markdown
Contributor

@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

@atomiks

atomiks commented Aug 23, 2024

Copy link
Copy Markdown
Contributor Author

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

@colmtuite

Copy link
Copy Markdown
Contributor

@atomiks I don't see the keepMounted prop documented in the component API, am I missing/misunderstanding something?

import { useEnhancedEffect } from '../../utils/useEnhancedEffect';
import { useCompositeListContext } from './CompositeListContext';

export interface UseCompositeListItemParameters {

Copy link
Copy Markdown
Member

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 ~

Copy link
Copy Markdown
Contributor Author

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

@atomiks
atomiks merged commit 0c87b6e into mui:master Sep 4, 2024
@atomiks
atomiks deleted the feat/RadioGroup branch September 4, 2024 03:51
@michaldudak michaldudak added component: radio group Changes related to the radio group component. and removed component: radio group labels May 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: radio group Changes related to the radio group component.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RadioGroup] Implement RadioGroup

5 participants