Skip to content

[Tooltip] Remove type="button" #1011

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

Merged
merged 2 commits into from
Dec 3, 2021
Merged

[Tooltip] Remove type="button" #1011

merged 2 commits into from
Dec 3, 2021

Conversation

jjenzz
Copy link
Contributor

@jjenzz jjenzz commented Dec 2, 2021

Closes #1010
Closes #993

I haven't done this for our other components that add type="button" since I don't believe we have had reports regarding those yet. Not sure what thoughts are here though?

@jjenzz jjenzz changed the title [Tooltip] Remove type="button" [Tooltip] Remove type="button" Dec 2, 2021
Copy link
Contributor

@benoitgrelard benoitgrelard left a comment

Choose a reason for hiding this comment

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

I haven't done this for our other components that add type="button" since I don't believe we have had reports regarding those yet. Not sure what thoughts are here though?

I have been wondering exactly the same thing. Trying to rationalise it, I feel like Tooltip.Trigger is the only trigger whose functionality isn't entirely relying on a click/pointerdown action. That makes me think that all the other triggers where we do type="button" have no business being replaced with other HTML elements as it wouldn't really be semantic anymore.

All that to say I think we're ok to do it just here for now and see what comes out on others if anything.

@jjenzz jjenzz force-pushed the tooltip-remove-type-button branch from 0b8a129 to 4a2b37b Compare December 3, 2021 13:50
@jjenzz jjenzz requested a review from benoitgrelard December 3, 2021 13:50
@jjenzz jjenzz merged commit 2d08b89 into main Dec 3, 2021
@jjenzz jjenzz deleted the tooltip-remove-type-button branch December 3, 2021 13:57
@bel0v
Copy link

bel0v commented Jan 24, 2024

Hey guys, found this PR when fixing a tooltip submitting a form. Just wondering — the issues mentioned seem to have a problem with an asChild usage of the Trigger. If asChild is not provided, the tooltip renders a button. So I didn't quite get it why this PR just removed type=button in all cases... Is there a case when a tooltip trigger as submit button is desired? 🤔

@benoitgrelard
Copy link
Contributor

This PR removed type=button because a tooltip trigger can be more than a button, it can sometimes be on a link, or even an input of many types.

@bel0v
Copy link

bel0v commented Mar 1, 2024

@benoitgrelard I get that, but that seems to be only the asChild use case. Otherwise the trigger renders a button, but now that button is of type submit.

@Roanmh
Copy link

Roanmh commented Mar 12, 2024

I just ran into this too. I wonder if the type=button parameter could be provided conditionally when the asChild property is false so the type property is guaranteed to be only be applied to a button component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tooltip] Trigger assumes type="button" [Tooltip] wrapping input element defaults its type to button
4 participants