[ButtonBase] Add nativeButton prop#47989
Conversation
Netlify deploy preview
Bundle size report
|
53c397d to
0396ac9
Compare
|
I am good with the new prop but we should have a clear example (ideally real scenario) to update the docs too. |
51fc75b to
b0f14e2
Compare
b0f14e2 to
2e14d7f
Compare
f84fa2b to
6b83890
Compare
6b83890 to
ae47aa4
Compare
| if (disabled) { | ||
| event.preventDefault(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
@siriwatknp Following up on the comment in #47985 (comment)
The additional click guard + return here would only affect anybody counting on programmatically focusing and clicking a disabled non-native ButtonBase-based component then also expecting the click handler to run, similar to #48003
9dc6ca8 to
0562586
Compare
0562586 to
f4b40dc
Compare
Added a brief section here ~ |
be48251 to
0e3a9a6
Compare
|
|
||
| #### Replacing native button elements with non-interactive elements | ||
|
|
||
| The `nativeButton` prop is available on `<ButtonBase>` and all button-like components to ensure that they are rendered with the correct HTML attributes before hydration, for example during server-side rendering. This should be specified when when passing a React component to the `component` prop of a button-like component. This should be specified if the custom component either replaces the default rendered element: |
There was a problem hiding this comment.
we can improve this phrasing, there's a double "when" and last 2 sentences start in the same way.
|
|
||
| ## Rendering non-native buttons | ||
|
|
||
| The `nativeButton` prop can be used to allow buttons to remain keyboard accessible when passing a React component to the [`component`](/material-ui/guides/composition/#passing-other-react-components) prop that renders a non-interactive element like a `<div>`. |
There was a problem hiding this comment.
For this, this information was not enough to understand fully what's going on. Maybe we need to have another example with the other case, when nativeButton is true and custom component is actually a button. Also, a an example when the prop is true and custom component returns not a button, we mention the warning.
There was a problem hiding this comment.
Maybe we need to have another example with the other case, when nativeButton is true and custom component is actually a button.
I think this is pretty rare in practice (e.g. making a MenuItem render a <button> elem) and not worth documenting, the dev warning should be self explanatory enough ~
an example when the prop is true and custom component returns not a button
Same here, the dev warnings should be enough for the user to correct the issue without having to go to the docs; only button -> span/div is worth documenting here as it's the more common use-case among the various dev warning combinations
| }, | ||
| }), | ||
| [], | ||
| [buttonRef], |
There was a problem hiding this comment.
should refs be included in deps array?
There was a problem hiding this comment.
It's just to satisfy eslint and has no runtime use, because the buttonRef is returned by the hook and isn't in the local scope anymore
| */ | ||
| LinkComponent: PropTypes.elementType, | ||
| /** | ||
| * Whether the custom component should render a native `<button>` element when |
There was a problem hiding this comment.
The custom component already renders whatever the user tells it to render. The nativeButton only allows non-buttons to retain button a11y. Or at least that's what I understood.
There was a problem hiding this comment.
Instead of should, how about Whether the custom component is expected to render…?
siriwatknp
left a comment
There was a problem hiding this comment.
Review comment on type prop handling.
| onTouchStart={handleTouchStart} | ||
| ref={handleRef} | ||
| tabIndex={disabled ? -1 : tabIndex} | ||
| type={type} |
There was a problem hiding this comment.
type={type} is set here as a direct JSX prop, and then buttonProps (from getButtonProps) is spread below — which also includes a resolved type (e.g. type="button" when nativeButton && !hasFormAction, or undefined otherwise).
Since the spread comes after, buttonProps.type will override the direct type={type} prop. This means the direct prop here is effectively dead code for non-link paths.
However, it also means the raw type prop (possibly undefined) does NOT take effect — the hook's resolved value always wins. Is that intentional?
If so, consider removing the direct type={type} to avoid confusion. If the intent is for the user's type to win, it should come after the spread.
There was a problem hiding this comment.
Removed in 0e5501b
For buttons the hook will set the type; for links, we add it to linkProps directly
# Conflicts: # packages/mui-material/src/Tab/Tab.test.js
silviuaavram
left a comment
There was a problem hiding this comment.
some comments, looks good! 🎉
|
|
||
| if (process.env.NODE_ENV !== 'production') { | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| React.useEffect(() => { |
There was a problem hiding this comment.
what's wrong with adding the check inside the hook and avoid the rules-of-hooks error?
There was a problem hiding this comment.
We do this everywhere, e.g. https://github.com/mui/material-ui/blob/master/packages/mui-material/src/Tooltip/Tooltip.js#L365-L372
I believe it's for bundlers to tree shake the entire effect to minimize bundle size
|
|
||
| const rootRef = React.useRef<HTMLElement | null>(null); | ||
| const focusableWhenDisabled = focusableWhenDisabledParam === true; | ||
| const { props: focusableWhenDisabledProps } = useFocusableWhenDisabled({ |
There was a problem hiding this comment.
why not just return focusableWhenDisabledProps instead of wrapping into another object?
There was a problem hiding this comment.
Flattened, originally was just in case we needed to return some non-props stuff
|
|
||
| const getButtonProps = React.useCallback( | ||
| <ExternalProps extends ButtonBaseExternalProps = ButtonBaseExternalProps>( | ||
| externalProps = EMPTY as ExternalProps, |
There was a problem hiding this comment.
In other cases I see we also default with {} instead of this EMPTY
There was a problem hiding this comment.
A micro-optimization to avoid creating the empty object more than once, it could be worthwhile to do a sweep through the whole lib to fix this
Currently this JSX
Renders this incorrect HTML: https://stackblitz.com/edit/7wy4fcg4?file=src%2FDemo.tsx
Continuing from #47985, the
component === 'button'heuristic doesn't work for whencomponentis a React component; an explicitnativeButton: booleanprop is required upfront to render the correct HTML, with the constraint that the attributes have to be resolved before mount (so there is no DOM to check)Who is affected and how:
A dev warning will be shown to users who are either:
<button>element (e.g.component={MySpan}), ordiv) with a component that resolves to a<button>elemente.g.
Button component={CustomButton}-> no warning if it resolves to<button>Button component={StyledSpan}-> warn unlessnativeButton={false}is specifiedMenuItem component={CustomLi}-> no warning if it doesn't resolve to<button>MenuItem component={CustomButton}-> warn unlessnativeButton={true}Affected components:
Button,Fab,IconButton,ListItemButton,MenuItem,StepButton,Tab,ToggleButton,AccordionSummary,BottomNavigationAction,CardActionArea,TableSortLabelinherit thenativeButtonprop via ButtonBasePaginationItemdoesn't useExtendButtonBaseTypeMapfor some reason so the type/proptype needed to be manually addedChipconditionally forwardsnativeButtondepending on whether it's a link (backward compatibility with "legacy" mode)Preview
Added a docs section in the
Buttonpage: https://deploy-preview-47989--material-ui.netlify.app/material-ui/react-button/#rendering-non-native-buttons