Skip to content
This repository was archived by the owner on Jan 20, 2022. It is now read-only.

AR-2016 Allow Tooltip to wrap a Button that is disabled #298

Merged
merged 2 commits into from
Jan 27, 2021

Conversation

justinanastos
Copy link
Contributor

@justinanastos justinanastos commented Jan 21, 2021

This works by capturing pointer events to signal that the cursor is over the button and then applying disabled styles directly to the button element and removing the disabled property so that tippy.js will show the tooltip. I went with this approach because then we can still test for disabled buttons.

AR-2016

Testing to see how this looks in Studio UI https://github.com/mdg-private/studio-ui/pull/3700

📦 Published PR as canary version: 8.12.1-canary.298.7907.0

✨ Test out this PR locally via:

npm install @apollo/space-kit@8.12.1-canary.298.7907.0
# or 
yarn add @apollo/space-kit@8.12.1-canary.298.7907.0

@justinanastos justinanastos added the minor Increment the minor version when merged label Jan 21, 2021
@justinanastos justinanastos force-pushed the justin/AR-2016/tooltip-on-disabled-button branch from 7b9cf6e to 81f5e35 Compare January 21, 2021 20:23
@justinanastos justinanastos requested a review from Jephuff January 21, 2021 20:31
@justinanastos justinanastos marked this pull request as ready for review January 21, 2021 20:31
@justinanastos justinanastos enabled auto-merge (squash) January 21, 2021 20:31
"onClick",
// If we're overriding the default disabled behavior, then strip
// it out from the props we'll pass to the element.
overrideDisabledBehavior ? "disabled" : "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an aria prop we should/could pass to make sure the button is still semantically disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only kind-of. We can use aria-disabled, but this will only provide special behavior for screen readers. I'll add it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ooh nice, thats probably where it matters most so thats perfect :)

@justinanastos justinanastos added the skip-release Preserve the current version when merged label Jan 25, 2021
@justinanastos justinanastos force-pushed the justin/AR-2016/tooltip-on-disabled-button branch 2 times, most recently from 8413d9e to 0973670 Compare January 25, 2021 21:19
… Tooltip

We'll need this so components can have special behavior if they live inside
of a Tooltip
@justinanastos justinanastos force-pushed the justin/AR-2016/tooltip-on-disabled-button branch from 0973670 to cafa14b Compare January 27, 2021 15:48
@justinanastos justinanastos requested a review from Jephuff January 27, 2021 15:52
@justinanastos justinanastos enabled auto-merge (squash) January 27, 2021 15:52
@justinanastos justinanastos removed the skip-release Preserve the current version when merged label Jan 27, 2021
"onClick",
// If we're overriding the default disabled behavior, then strip
// it out from the props we'll pass to the element.
overrideDisabledBehavior ? "disabled" : "",
Copy link
Contributor

Choose a reason for hiding this comment

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

ooh nice, thats probably where it matters most so thats perfect :)

@justinanastos justinanastos merged commit 0efca1a into main Jan 27, 2021
@justinanastos justinanastos deleted the justin/AR-2016/tooltip-on-disabled-button branch January 27, 2021 15:56
@apollo-bot2
Copy link
Collaborator

🚀 PR was released in v8.13.0 🚀

@apollo-bot2 apollo-bot2 added the released This issue/pull request has been released. label Jan 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
minor Increment the minor version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants