-
-
Notifications
You must be signed in to change notification settings - Fork 300
[useButton] nativeButton prop
#1909
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
Conversation
commit: |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| @@ -1,7 +1,5 @@ | |||
| 'use client'; | |||
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 think this hook was for in case we ever wanted to expose the element/tag name as a prop for other components, since we're not doing that it can just be dropped and merged into 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.
The event handler still uses it inside the events for e.g. event.preventDefault()
|
We don't really need to know the actual rendered tag name, but just the fact that they want to render something different than the native |
|
@michaldudak it also checks for base-ui/packages/react/src/use-button/useButton.ts Lines 59 to 60 in 1634afe
|
|
I don't think we should treat links the same way as buttons. I'd rather have separate |
|
Seems fair enough. Our components usually have a |
disabled attribute is present during SSRnativeButton prop
|
@michaldudak updated PR description Adds |
99db41e to
3a4f135
Compare
3a4f135 to
2091c45
Compare
Bundle size reportTotal Size Change: @base-ui-components/react parsed: Show 8 more bundle changes@base-ui-components/react/collapsible parsed: |
| buttonRef: forwardedRef, | ||
| elementName: 'a', | ||
| native: 'a', | ||
| focusableWhenDisabled: true, |
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.
Links cannot be disabled, thats why focusableWhenDisabled wasn't passed here; I realize the TOOLBAR_LINK_METADATA above is super misleading, but the true is only used for deriving disabledIndices
I added a comment about this here, sorry for conflicting this PR again btw 🙏
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 also remove support for links in useButton (could be in a separate PR) and create another hook if necessary.
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 agree to do it separately; at a glance there's actually nothing to put in a useLink hook and we can just remove the link logic
ToolbarLink is the only link component using this; the only other link component we have is NavigationMenu.Link and that doesn't use useButton
| buttonRef: forwardedRef, | ||
| elementName: 'a', | ||
| native: 'a', | ||
| focusableWhenDisabled: true, |
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 also remove support for links in useButton (could be in a separate PR) and create another hook if necessary.
| "nativeButton": { | ||
| "type": "boolean", | ||
| "default": "true", | ||
| "description": "Determines whether the component is being rendered as a native 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.
IMO we need a more detailed description what it's for. How about adding "Set it to false if the render prop renders a non-interactive element, like div or span."?
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.
It can technically be replaced with an interactive element, so I think any different tag should be the doc
| if (hasError) { | ||
| throw new Error( | ||
| `Set the \`nativeButton\` prop to \`${isButtonButFalse}\` when rendering a <${tagNameLower}> element.`, | ||
| ); | ||
| } | ||
| }, [native]); |
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.
Since this prop is quite obscure and not shown in the types when using render by default, a runtime error is likely required
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.
Nevermind, doesn't play nicely with the conformance tests
Fixes #1843
Fixes #2117
Adds
nativeButton: booleanprop to components that useuseButton.When using the
renderprop, you must specifynativeButton={boolean}if the tag that's rendered differs from the default one (these are documented already asRenders a <button> elementorRenders a <div> element).