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

[ButtonBase][material-next] Add ButtonBase component #38319

Merged
merged 19 commits into from
Aug 22, 2023

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Aug 4, 2023

Summary

This PR adds the ButtonBase component to material-next and refactors the existing Button accordingly.

Why bring back ButtonBase

Base UI’s useButton extracts most of the logic for which ButtonBase was used in v5, but some things are material-specific or style-specific that it doesn't:

  • Adding the ripple (reference)
  • Adding the focusVisible imperative handle (reference)
  • Resetting the button's element default styles (reference)
  • Changing to the anchor element when href (or to) is provided (reference)

These four things will be shared by the “button-like” components: Button, FAB, IconButton, Tabs, and clickable Chip. This is why I propose bringing back the ButtonBase component.

Keeping ButtonBase has two additional beneficial effects:

  • It will reduce breaking changes and help keep the API stable from v5 to v6. This is important as v6 is expected to carry significant changes like using Base UI, Zero-runtime CSS, and Material You styles.
  • It will simplify migrating the “button-like” components, as the Base UI adoption is encapsulated in ButtonBase.

It’s essential to keep in mind that it shouldn’t be overused. For example, if a component only needs the ripple, it should use the ripple hooks and components directly, not ButtonBase. This way, we will keep ButtonBase's scope controlled.

Changes

  • Add the ButtonBase component, based on Base UI’s useButton:
    • Types are the same as ButtonBase v5
    • Tests have a couple of changes:
      • Removed focusRipple and ripple’s pulsate tests as Material You no longer used a ripple for the focused state (benchmark).
      • In v5, a disabled ButtonBase would have a tabIndex = -1 by default. This is not maintained in the useButton hook (test change)
      • For preventing onKeyUp default, muiPreventDefault is used (change here)
  • Refactor the Button component to use ButtonBase
  • Move ripple code from the Button folder to the ButtonBase folder.

Important note for reviewers

The first commit copies files from v5, so if you want to review actual changes, exclude that commit when looking at the files. For some reason, the only file this isn't working for is ButtonBase.tsx; sorry for that.

Questions for reviewers

  • The touchRippleRef prop was deleted when the Button was added to material-next. When adding ButtonBase, I kept it. Do you think we should remove it?
  • Should the change regarding tabIndex = -1 not added when the button is disabled be a breaking change? I don’t think it is, but maybe I’m missing something.
  • Is the muiPreventDefault breaking change explanation correct?

@DiegoAndai DiegoAndai added component: ButtonBase The React component. v6.x labels Aug 4, 2023
@DiegoAndai DiegoAndai self-assigned this Aug 4, 2023
@mui-bot
Copy link

mui-bot commented Aug 4, 2023

Netlify deploy preview

https://deploy-preview-38319--material-ui.netlify.app/

@mui/material-next: parsed: +0.24% , gzip: +0.15%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against ed2632d

@DiegoAndai DiegoAndai force-pushed the material-next-button-base branch from bb835d6 to e387775 Compare August 4, 2023 18:59
@DiegoAndai DiegoAndai force-pushed the material-next-button-base branch from e387775 to b236c84 Compare August 4, 2023 19:13
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 9, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 9, 2023
@DiegoAndai DiegoAndai marked this pull request as ready for review August 14, 2023 19:50
packages/mui-material-next/migration.md Outdated Show resolved Hide resolved
Co-authored-by: Albert Yu <[email protected]>
Signed-off-by: Diego Andai <[email protected]>
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks good, left few comments for consideration before merging

packages/mui-material-next/migration.md Show resolved Hide resolved
@@ -1,43 +1,22 @@
'use client';
Copy link
Member

Choose a reason for hiding this comment

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

image
❤️

@mnajdova
Copy link
Member

Should the change regarding tabIndex = -1 not added when the button is disabled be a breaking change? I don’t think it is, but maybe I’m missing something.

Considering we are now using Base UI, we have support for whether the button should be focusable when disabled which should allow developers to setup the button anyway that works for them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ButtonBase The React component. v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants