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

Jump to bottom

[ToggleButton][ToggleButtonGroup] Create ToggleButton and ToggleButtonGroup #763

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open

[ToggleButton][ToggleButtonGroup] Create ToggleButton and ToggleButtonGroup #763

wants to merge 32 commits into from

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Oct 28, 2024 •
edited
Loading

@mj12albert mj12albert added new feature New feature or request component: toggle button This is the name of the generic UI component, not the React module! labels Oct 28, 2024
@mui-bot
Copy link

mui-bot commented Oct 28, 2024 •
edited
Loading

Netlify deploy preview

https://deploy-preview-763--base-ui.netlify.app/

Generated by 🚫 dangerJS against f715087

@mj12albert mj12albert force-pushed the feat/toggle-button branch 4 times, most recently from 6c0fe92 to 6e04229 Compare October 28, 2024 17:06
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 30, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 30, 2024
@mj12albert mj12albert force-pushed the feat/toggle-button branch 5 times, most recently from 9bcba83 to 3a74374 Compare October 31, 2024 14:07
@mj12albert mj12albert changed the title [ToggleButton] Create ToggleButton [ToggleButton][ToggleButtonGroup] Create ToggleButton and ToggleButtonGroup Oct 31, 2024
@mj12albert mj12albert force-pushed the feat/toggle-button branch 2 times, most recently from 7cc24fd to b06a97d Compare October 31, 2024 15:17
@mj12albert mj12albert added the component: button group This is the name of the generic UI component, not the React module! label Oct 31, 2024
@mj12albert mj12albert force-pushed the feat/toggle-button branch 10 times, most recently from bcdd1cc to 7c449d8 Compare November 1, 2024 07:52
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 21, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 21, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 25, 2024
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Nov 25, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 27, 2024
mj12albert
mj12albert commented Nov 27, 2024
View reviewed changes
packages/react/src/composite/root/useCompositeRoot.ts
Comment on lines +108 to +111
if (textDirectionRef?.current == null) {
textDirectionRef.current = getTextDirection(element);
}
const isRtl = textDirectionRef.current === 'rtl';
Copy link
Member Author

Choose a reason for hiding this comment

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

Caching this in a ref to mitigate against spamming the keys raised in #831 (comment)

@mj12albert mj12albert mentioned this pull request Nov 27, 2024
Open
@mj12albert
Copy link
Member Author

mj12albert commented Nov 28, 2024 •
edited
Loading

This PR is up to date again with all the library changes since it was opened, pinging @colmtuite again for a review 🙏

michaldudak
michaldudak reviewed Nov 28, 2024
View reviewed changes
packages/react/src/toggle-button-group/item/ToggleButtonGroupItem.tsx
*
* - [ToggleButtonGroupItem API](https://base-ui.com/components/react-toggle-button-group/#api-reference-ToggleButtonGroupItem)
*/
const ToggleButtonGroupItem = React.forwardRef(function ToggleButtonGroupItem(
Copy link
Member

Choose a reason for hiding this comment

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

We should make the API consistent with RadioGroup and CheckboxGroup - they don't have the .Item part but use Radio.Root and Checkbox.Root

Copy link
Member Author

@mj12albert mj12albert Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I started off doing that but in the end it was more practical to have this Item subcomponent, let me look up what the actual problem was

@colmtuite is this an issue for you?

packages/react/src/toggle-button/root/useToggleButtonRoot.ts
}

export namespace useToggleButtonRoot {
export interface Parameters {
Copy link
Member

Choose a reason for hiding this comment

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

Make the hook parameters mandatory (per #856). Defaults should be set on the component level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated ~ f715087

packages/react/src/toggle-button-group/root/useToggleButtonGroupRoot.ts
* The value of the ToggleButtonGroup represented by an array of values
* of the items that are pressed
*/
value: readonly string[];
Copy link
Member

Choose a reason for hiding this comment

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

It might be better DX if the type of the value was an array only when toggleMultiple is set.
For parity with RadioGroup, we should allow any type of value.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the value, I think it might be easier to remember (or harder to do something wrong) if it was always an array, if not we'd have to do a warning or something if you get the type wrong here

What do you think @atomiks @vladmoroz ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atomiks atomiks atomiks approved these changes

@michaldudak michaldudak michaldudak left review comments

@colmtuite colmtuite Awaiting requested review from colmtuite colmtuite is a code owner

Assignees
No one assigned
Labels
component: toggle button This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ToggleButtonGroup] Implement ToggleButtonGroup [ToggleButton] Implement ToggleButton
4 participants
@mj12albert @mui-bot @michaldudak @atomiks