-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
[Button] Create ButtonUnstyled and useButton #27600
Conversation
Details of bundle changes (experimental) @material-ui/core: parsed: +0.63% , gzip: +0.59% |
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.
I believe we could try to use it in the ButtonBase
, at least as an experiment.
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.
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)
@@ -0,0 +1,51 @@ | |||
import * as React from 'react'; | |||
import Stack from '@material-ui/core/Stack'; |
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.
Off-topic. It's going to funny once we have:
import Stack from '@material-ui/core/Stack'; | |
import Stack from '@mui/core-material/Stack'; |
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.
I suppose the layout components should live in either the System or Base package.
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.
Could we use useButton in ButtonBase?
Yes, I believe there are no changes that would prevent it.
@@ -0,0 +1,51 @@ | |||
import * as React from 'react'; | |||
import Stack from '@material-ui/core/Stack'; |
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.
I suppose the layout components should live in either the System or Base package.
Needs to be rebased on |
@mui-org/core I need your feedback on handling events by 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 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
Outdated
Show resolved
Hide resolved
Re my previous comment: In a discussion with @mnajdova we decided it'll be better if the handlers passed in the |
6763297
to
75c5c0b
Compare
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.
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
Outdated
Show resolved
Hide resolved
const ownHandlers = { | ||
onClick: (_: any, next: () => void) => { | ||
callLog.push('own'); | ||
next(); |
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.
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.
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.
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
.
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.
As pointed out in #27600 (comment): Let's go without abstraction first to move this along.
packages/material-ui-unstyled/src/utils/extractEventHandlers.ts
Outdated
Show resolved
Hide resolved
packages/material-ui-unstyled/src/utils/extractEventHandlers.ts
Outdated
Show resolved
Hide resolved
packages/material-ui-unstyled/src/utils/extractEventHandlers.ts
Outdated
Show resolved
Hide resolved
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.
"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.
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.
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 🤔.
ButtonUnstyledProps, | ||
buttonUnstyledClasses, | ||
} from '@material-ui/unstyled/ButtonUnstyled'; | ||
import { Theme, ThemeProvider, createTheme } from '@material-ui/core/styles'; |
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.
Is there a way we could remove the import from the soon to be named material package? I'm not sure.
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.
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.
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.
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.
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.
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
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.
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.
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.
Let's move the discussion to #28073
return <ButtonUnstyled {...props} component={CustomButtonRoot} />; | ||
} | ||
|
||
export default function UnstyledButton() { |
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.
export default function UnstyledButton() { | |
export default function UnstyledButtonsSimple() { |
return <ButtonUnstyled {...props} component={CustomButtonRoot} />; | ||
} | ||
|
||
export default function UnstyledButton() { |
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.
export default function UnstyledButton() { | |
export default function UnstyledButtonsSpan() { |
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.
🤦♂️ I overlooked this again, sorry.
|
||
{{"demo": "pages/components/buttons/UnstyledButtonsSpan.js"}} | ||
|
||
Compare the attributes on the span with the button from the previous demo |
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.
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. |
() => ({ | ||
focusVisible: () => { | ||
setFocusVisible(true); | ||
buttonRef?.current?.focus(); |
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.
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?
buttonRef?.current?.focus(); | |
buttonRef.current!.focus(); |
@@ -2,14 +2,13 @@ import * as React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
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'; |
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.
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.
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.
Sure, it's not a big deal for us and if it can improve DX, then let's do it.
@@ -180,3 +180,46 @@ However: | |||
``` | |||
|
|||
This has the advantage of supporting any element, for instance, a link `<a>` element. | |||
|
|||
## Unstyled button |
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.
Should we normalize the headers into
## Unstyled button | |
## Unstyled |
Or keep Unstyled COMPONENT NAME
?
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.
I'd say it's simpler to have it without the component name.
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.
I have asked because we have done it in two different ways:
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.
Yeah, let's make it consistent without the component name
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.
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.
ButtonUnstyledProps, | ||
buttonUnstyledClasses, | ||
} from '@material-ui/unstyled/ButtonUnstyled'; | ||
import { Theme, ThemeProvider, createTheme } from '@material-ui/core/styles'; |
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.
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.
return <ButtonUnstyled {...props} component={CustomButtonRoot} />; | ||
} | ||
|
||
export default function UnstyledButton() { |
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.
🤦♂️ I overlooked this again, sorry.
@@ -180,3 +180,46 @@ However: | |||
``` | |||
|
|||
This has the advantage of supporting any element, for instance, a link `<a>` element. | |||
|
|||
## Unstyled button |
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.
Yeah, let's make it consistent without the component name
@@ -2,14 +2,13 @@ import * as React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
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'; |
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.
Sure, it's not a big deal for us and if it can improve DX, then let's do it.
@oliviertassinari All the discussed changes are in #28074 |
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 callpreventDefault
on thekeydown
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