[Button] Create ButtonUnstyled and useButton by michaldudak · Pull Request #27600 · mui/material-ui

michaldudak · 2021-08-04T10:41:29Z

This adds the ButtonUnstyled and useButton to the Unstyled package.

It contains mostly the code from ButtonBase. One notable addition is related to the active state. The original implementation encouraged the use of the :active pseudo-class. However, because of #19784, when the underlying component is not a native button or link anchor, we call preventDefault on the keydown event, thus preventing the active state to be applied on the element. The new solution correctly recognizes when an element activated by the space key is active.

Preview: https://deploy-preview-27600--material-ui.netlify.app/components/buttons

One chunk of mui/base-ui#10

mui-pr-bot · 2021-08-04T10:43:29Z

Details of bundle changes (experimental)

@material-ui/core: parsed: +0.63% , gzip: +0.59%
@material-ui/lab: parsed: +0.45% , gzip: +0.39%
@material-ui/unstyled: parsed: +7.84% , gzip: +6.49%

Generated by 🚫 dangerJS against 66019df

mnajdova

I believe we could try to use it in the ButtonBase, at least as an experiment.

docs/src/pages/components/buttons/UnstyledButtons.js

docs/src/pages/components/buttons/buttons.md

docs/src/pages/components/buttons/UnstyledButtons.js

oliviertassinari

Could we use useButton in ButtonBase? It would allow to:

Increase useButton test coverage
Make sure we didn't break ButtonBase tests
Make sure we can remove most of the logic of ButtonBase (that the hook is doing what's its meant to)
Increase developers' trust (they are not using some uncompletely battle-tested logic)

test/utils/describeConformanceUnstyled.tsx

docs/src/pages/components/buttons/buttons.md

docs/src/pages/components/buttons/UseButton.tsx

docs/src/pages/components/buttons/UseButton.js

oliviertassinari · 2021-08-08T23:10:36Z

docs/src/pages/components/buttons/UnstyledButtons.tsx

@@ -0,0 +1,51 @@
+import * as React from 'react';
+import Stack from '@material-ui/core/Stack';


Off-topic. It's going to funny once we have:

Suggested change

import Stack from '@material-ui/core/Stack';

import Stack from '@mui/core-material/Stack';

I suppose the layout components should live in either the System or Base package.

docs/src/pages/components/buttons/buttons.md

michaldudak

Could we use useButton in ButtonBase?

Yes, I believe there are no changes that would prevent it.

michaldudak · 2021-08-09T07:34:42Z

docs/src/pages/components/buttons/UnstyledButtons.tsx

@@ -0,0 +1,51 @@
+import * as React from 'react';
+import Stack from '@material-ui/core/Stack';


I suppose the layout components should live in either the System or Base package.

docs/src/pages/components/buttons/UseButton.js

docs/src/pages/components/buttons/UseButton.tsx

docs/src/pages/components/buttons/buttons.md

eps1lon · 2021-08-10T09:10:49Z

Needs to be rebased on next (b664deb) before merge.

docs/src/pages/components/buttons/UnstyledButtonCustom.tsx

michaldudak · 2021-08-13T09:00:32Z

@mui-org/core I need your feedback on handling events by useButton (and possibly other hooks in the future).
An exemplary usage of the hook looks like this:

function CustomButton(props) {
  const button = useButton(props);

  const otherHandlers = {
    onClick: () => { 
      if (!button.disabled) console.log('clicked');
    }
  }

  return (
    <button {...button.getRootProps(otherHandlers)}>Button</button>
  );
}

The event handlers may be passed in in two ways: as props supplied to useButton, or as an argument to getRootProps. The second option is provided so that CustomButton can set its own handlers that take into consideration the return value of useButton.
To make this work, useButton calls its internal event handlers first and then (if appropriate), the ones passed in to getRootProps, and finally the ones passed in the props. The code responsible for chaining these handlers is in https://github.com/mui-org/material-ui/pull/27600/files#diff-82ce2e0538c06ca9335627e5bf0a896699750b85546a41598ac0c36b8e56a41b.

The useButton's internal handlers have a way to decide at which point call other handlers. The ones from getRootProps and props are called one after the other.

Please let me know if this approach is sensible for you.

packages/material-ui-unstyled/src/ButtonUnstyled/useButton.test.tsx

michaldudak · 2021-08-13T09:50:30Z

Re my previous comment: In a discussion with @mnajdova we decided it'll be better if the handlers passed in the getRootProps need to explicitly call handlers from props, as it'll create less confusion for developers.

eps1lon

Let's go without the mergeEventHandler abstraction first and discuss it separately to get a clearer picture what it is for and how it should be used.

Implementation and abstraction in the same PR makes review needlessly hard and makes it impossible later to determine what we were actually abstracting.

"Root" is also a MUI related term that's not really appropriate here. People can come up with a different definition for it. What you probably mean is getHostProps (i.e. the props for the element that will hold all the semantics e.g. the one with [role="button"]) instead of getRootProps

packages/material-ui-unstyled/src/utils/mergeEventHandlers.test.ts

eps1lon · 2021-08-17T13:49:07Z

packages/material-ui-unstyled/src/utils/mergeEventHandlers.test.ts

+    const ownHandlers = {
+      onClick: (_: any, next: () => void) => {
+        callLog.push('own');
+        next();


This is not how event handlers work. They always call the other handlers. Immediate propagation and default prevented need to be called explicitly. In other words, chaining is always opt-out not opt-in.

In case of the useButton (and, specifically, using it to implement ButtonBase) we need control over when the dev-supplied event handler gets called (see https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/ButtonBase/ButtonBase.js#L216 for example). That's why I introduced the callback parameter to the handlers internal to useButton.

As pointed out in #27600 (comment): Let's go without abstraction first to move this along.

packages/material-ui-unstyled/src/ButtonUnstyled/useButton.ts

packages/material-ui-unstyled/src/utils/extractEventHandlers.ts

michaldudak

"Root" is also a MUI related term that's not really appropriate here. People can come up with a different definition for it. What you probably mean is getHostProps (i.e. the props for the element that will hold all the semantics e.g. the one with [role="button"]) instead of getRootProps

@eps1lon I meant the Root in MUI terms here. These are the props to be placed on an element that's at the root of the component's element tree. This is equivalent to the Root slot of the unstyled or Core Button.
getHostProps is also a good name in case of a button, but makes less sense in more complex components. For example useAutocomplete, being much more complex, has many more "slot" methods: https://github.com/mui-org/material-ui/blob/next/packages/material-ui-unstyled/src/AutocompleteUnstyled/useAutocomplete.js#L985

Let's go without the mergeEventHandler abstraction first and discuss it separately to get a clearer picture what it is for and how it should be used.

This is now gone.

docs/src/pages/components/buttons/buttons.md

oliviertassinari

I had a new look at the PR, happy to see the changes in :)

The size of the Button seems to have increased by 0.5 kB gzipped. I think that it will be interesting to keep an eye of the overhead of building the styled version with the unstyled, and not the hook one. It almost feels like building the styled version with the hook is enough and simpler 🤔.

oliviertassinari · 2021-08-30T12:52:54Z

docs/src/pages/components/buttons/UnstyledButtonCustom.tsx

+  ButtonUnstyledProps,
+  buttonUnstyledClasses,
+} from '@material-ui/unstyled/ButtonUnstyled';
+import { Theme, ThemeProvider, createTheme } from '@material-ui/core/styles';


Is there a way we could remove the import from the soon to be named material package? I'm not sure.

Good point. When imported from @material-ui/system, the theme got from createTheme() doesn't have the defaults, so it would be necessary to create all the used tokens. Alternatively, I'm thinking I could rely on just the theme.palette.mode and use hardcoded colors in CSS.

I honestly don't know what would be best. The current tradeoff might already be great.
Another option would be to use the color palette of Joy, if the defaut theme comes fully loaded with it.

Or, use the palette from the docs.
However, not relying on theme values would make the demo look the same everywhere. So if someone just takes the demo code and pastes it in their project, they won't be confused that it looks different.
Here's a new implementation - let me know if you like it better (it has not changed visually): michaldudak@40045aa

cc @siriwatknp and @danilo-leal for direction on this.

So if someone just takes the demo code and pastes it in their project, they won't be confused that it looks different.

I had this in mind with the palette color of Joy, say if it comes with blue, red, yellow, purple, orange, etc. by default.

Let's move the discussion to #28073

oliviertassinari · 2021-08-30T13:01:59Z

docs/src/pages/components/buttons/UnstyledButtonsSimple.tsx

+  return <ButtonUnstyled {...props} component={CustomButtonRoot} />;
+}
+
+export default function UnstyledButton() {


Suggested change

export default function UnstyledButton() {

export default function UnstyledButtonsSimple() {

oliviertassinari · 2021-08-30T13:03:53Z

docs/src/pages/components/buttons/UnstyledButtonsSpan.tsx

+  return <ButtonUnstyled {...props} component={CustomButtonRoot} />;
+}
+
+export default function UnstyledButton() {


Suggested change

export default function UnstyledButton() {

export default function UnstyledButtonsSpan() {

🤦‍♂️ I overlooked this again, sorry.

docs/src/pages/components/buttons/buttons.md

oliviertassinari · 2021-08-30T13:07:00Z

docs/src/pages/components/buttons/buttons.md

+
+{{"demo": "pages/components/buttons/UnstyledButtonsSpan.js"}}
+
+Compare the attributes on the span with the button from the previous demo


Suggested change

Compare the attributes on the span with the button from the previous demo

Compare the attributes on the span with the button from the previous demo.

oliviertassinari · 2021-08-30T13:12:31Z

packages/material-ui-unstyled/src/ButtonUnstyled/ButtonUnstyled.tsx

+    () => ({
+      focusVisible: () => {
+        setFocusVisible(true);
+        buttonRef?.current?.focus();


I can't remember the last time I saw a valide usage of ? with React ref. Here, the first one seems unnecessary since it's coming from a React.useRef, the second one seems unnecessary since refs resolve before imperative handle. Would it make more sense with?

Suggested change

buttonRef?.current?.focus();

buttonRef.current!.focus();

oliviertassinari · 2021-08-30T13:18:56Z

packages/material-ui/src/ButtonBase/ButtonBase.js

 import clsx from 'clsx';
 import { elementTypeAcceptingRef, refType } from '@material-ui/utils';
-import { unstable_composeClasses as composeClasses } from '@material-ui/unstyled';
+import { unstable_composeClasses as composeClasses, useButton } from '@material-ui/unstyled';


I wonder if we shouldn't update all the unstyled imports to go one level deep for improving the DX when using a single component. With a growing size of unstyled, it starts to be increasingly interesting.

Sure, it's not a big deal for us and if it can improve DX, then let's do it.

oliviertassinari · 2021-08-30T13:25:19Z

docs/src/pages/components/buttons/buttons.md


 This has the advantage of supporting any element, for instance, a link `<a>` element.
+
+## Unstyled button


Should we normalize the headers into

Suggested change

## Unstyled button

## Unstyled

Or keep Unstyled COMPONENT NAME?

I'd say it's simpler to have it without the component name.

I have asked because we have done it in two different ways:

https://next.material-ui.com/components/modal/#unstyled

https://next.material-ui.com/components/switches/#unstyled-switches

Yeah, let's make it consistent without the component name

michaldudak

The size of the Button seems to have increased by 0.5 kB gzipped. I think that it will be interesting to keep an eye of the overhead of building the styled version with the unstyled, and not the hook one. It almost feels like building the styled version with the hook is enough and simpler 🤔.

In v6, w could reduce the size as the Button could use useButton directly, without the intermediary ButtonBase. I think ButtonBase could be removed completely then.

michaldudak · 2021-08-31T11:38:03Z

docs/src/pages/components/buttons/UnstyledButtonCustom.tsx

+  ButtonUnstyledProps,
+  buttonUnstyledClasses,
+} from '@material-ui/unstyled/ButtonUnstyled';
+import { Theme, ThemeProvider, createTheme } from '@material-ui/core/styles';


Good point. When imported from @material-ui/system, the theme got from createTheme() doesn't have the defaults, so it would be necessary to create all the used tokens. Alternatively, I'm thinking I could rely on just the theme.palette.mode and use hardcoded colors in CSS.

michaldudak · 2021-08-31T11:41:50Z

docs/src/pages/components/buttons/UnstyledButtonsSpan.tsx

+  return <ButtonUnstyled {...props} component={CustomButtonRoot} />;
+}
+
+export default function UnstyledButton() {


🤦‍♂️ I overlooked this again, sorry.

michaldudak · 2021-08-31T11:42:35Z

docs/src/pages/components/buttons/buttons.md


 This has the advantage of supporting any element, for instance, a link `<a>` element.
+
+## Unstyled button


Yeah, let's make it consistent without the component name

michaldudak · 2021-08-31T12:10:05Z

packages/material-ui/src/ButtonBase/ButtonBase.js

 import clsx from 'clsx';
 import { elementTypeAcceptingRef, refType } from '@material-ui/utils';
-import { unstable_composeClasses as composeClasses } from '@material-ui/unstyled';
+import { unstable_composeClasses as composeClasses, useButton } from '@material-ui/unstyled';


Sure, it's not a big deal for us and if it can improve DX, then let's do it.

michaldudak · 2021-08-31T14:46:09Z

@oliviertassinari All the discussed changes are in #28074

michaldudak added 6 commits July 30, 2021 15:29

Add ButtonUnstyled

8e9e330

useButton

4711e17

Move props to a separate file

c5f12ea

Move logic to useButton

aad757a

Make useButton props consistent with ButtonUnstyled

8a11765

Add API docs and demos

858eca8

michaldudak added scope: button Changes related to the button. package: @mui/base Specific to @mui/base (legacy). labels Aug 4, 2021

github-actions bot mentioned this pull request Aug 4, 2021

Update snapshots for pr/27600 eps1lon/mui-scripts-incubator#2908

Closed

michaldudak added 2 commits August 4, 2021 14:04

Fix the build

11359b7

Format demos

acbd594

michaldudak mentioned this pull request Feb 27, 2024

Base UI components progress mui/base-ui#10

Closed

27 tasks

mnajdova reviewed Aug 5, 2021

View reviewed changes

docs/src/pages/components/buttons/UnstyledButtons.js Outdated Show resolved Hide resolved

docs/src/pages/components/buttons/buttons.md Outdated Show resolved Hide resolved

docs/src/pages/components/buttons/UnstyledButtons.js Outdated Show resolved Hide resolved

michaldudak added 4 commits August 5, 2021 16:32

Add better support for the active state

7290dee

Add useButton tests

4f10ac0

Merge remote-tracking branch 'upstream/next' into feat/unstyled-button

ee1ae90

Write API docs

f21564a

oliviertassinari reviewed Aug 8, 2021

View reviewed changes

docs/src/pages/components/buttons/buttons.md Show resolved Hide resolved

michaldudak commented Aug 9, 2021

View reviewed changes

michaldudak added 2 commits August 9, 2021 14:03

Add more demos

6b33819

Merge remote-tracking branch 'upstream/next' into feat/unstyled-button

c2ee127

michaldudak added 6 commits August 12, 2021 13:36

Use useButton in ButtonBase

06d4b3c

Merge remote-tracking branch 'upstream/next' into feat/unstyled-button

68b06ab

Fix tests

08d8f41

Use useTouchRipple in ButtonBase

b85648c

Extract chainEventHandlers to utils

d3be252

Merge branch 'next' into feat/unstyled-button

fcee0fb

eps1lon reviewed Aug 13, 2021

View reviewed changes

docs/src/pages/components/buttons/UnstyledButtonCustom.tsx Outdated Show resolved Hide resolved

mnajdova reviewed Aug 13, 2021

View reviewed changes

packages/material-ui-unstyled/src/ButtonUnstyled/useButton.test.tsx Outdated Show resolved Hide resolved

michaldudak added 5 commits August 13, 2021 12:51

Refactor and correct useButton and demos

065052a

Do not call all event handlers automatically

4d1fcc9

Merge branch 'next' into feat/unstyled-button

a46d7c1

Add dark mode for adv customisation demo

52ec0a6

Fix regression tests

75c5c0b

michaldudak force-pushed the feat/unstyled-button branch from 6763297 to 75c5c0b Compare August 15, 2021 21:10

eps1lon requested changes Aug 17, 2021

View reviewed changes

Apply CR suggestions

4586840

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Aug 18, 2021

Merge remote-tracking branch 'upstream/next' into feat/unstyled-button

ce874d9

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Aug 20, 2021

michaldudak commented Aug 20, 2021

View reviewed changes

docs/src/pages/components/buttons/buttons.md Show resolved Hide resolved

michaldudak added 3 commits August 20, 2021 11:45

Do not dispatch a click event

df52895

Remove mergeEventHandlers utility

41858c1

Tweak transition speed in demos

66019df

michaldudak requested a review from eps1lon August 30, 2021 09:26

eps1lon approved these changes Aug 30, 2021

View reviewed changes

michaldudak merged commit 5f30983 into mui:next Aug 30, 2021

michaldudak deleted the feat/unstyled-button branch August 30, 2021 11:12

oliviertassinari reviewed Aug 30, 2021

View reviewed changes

michaldudak commented Aug 31, 2021

View reviewed changes

This was referenced Aug 31, 2021

[docs] Don't use Material theme in unstyled demos #28073

Merged

[Button] Use deeper imports from unstyled, correct docs #28074

Merged

oliviertassinari mentioned this pull request Sep 5, 2021

[website] Add product-x page #28106

Merged

1 task

michaldudak mentioned this pull request Sep 8, 2021

[ButtonBase] Revert to the pre-unstyled implementation #28225

Merged

		@@ -0,0 +1,51 @@
		import * as React from 'react';
		import Stack from '@material-ui/core/Stack';

	import Stack from '@material-ui/core/Stack';
	import Stack from '@mui/core-material/Stack';

	export default function UnstyledButton() {
	export default function UnstyledButtonsSimple() {


		{{"demo": "pages/components/buttons/UnstyledButtonsSpan.js"}}

		Compare the attributes on the span with the button from the previous demo


		This has the advantage of supporting any element, for instance, a link `<a>` element.

		## Unstyled button

Uh oh!

Conversation

michaldudak commented Aug 4, 2021 • edited by oliviertassinari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mui-pr-bot commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnajdova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oliviertassinari left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaldudak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eps1lon commented Aug 10, 2021

Uh oh!

Uh oh!

michaldudak commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

michaldudak commented Aug 13, 2021

Uh oh!

eps1lon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaldudak left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oliviertassinari left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaldudak commented Aug 4, 2021 •

edited by oliviertassinari

Loading

mui-pr-bot commented Aug 4, 2021 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

michaldudak commented Aug 13, 2021 •

edited

Loading

michaldudak left a comment •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari Aug 30, 2021 •

edited

Loading