Skip to content
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

Overlay on disabled elements, to catch events and show tooltips #27529

Merged
merged 22 commits into from
Jan 21, 2021

Conversation

grzim
Copy link
Contributor

@grzim grzim commented Dec 5, 2020

Description

Closes #27451
According to the issue #27451 - there was no mechanism for disabled elements to catch events and to display a tooltip.
I have added a layer placed above disabled elements that work as an event catcher and therefore make a tooltip work.

I wouldn't say it is a bug fix, rather a change in current behavior. In my opinion, this change would benefit UX.

How has this been tested?

Unit test added to cover this scenario.
Manual test.

Screenshots

alt text

Types of changes

Current behaviour altered

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@grzim grzim changed the title Overlay on disabled elements, to catch evetns and show tooltips Overlay on disabled elements, to catch events and show tooltips Dec 5, 2020
Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

Due to this being a change in interaction at a component level I'll also add two apropiate labels.

packages/components/src/tooltip/index.js Outdated Show resolved Hide resolved
packages/components/src/tooltip/index.js Outdated Show resolved Hide resolved
packages/components/src/tooltip/index.js Outdated Show resolved Hide resolved
packages/components/src/tooltip/test/index.js Outdated Show resolved Hide resolved
packages/components/src/tooltip/index.js Outdated Show resolved Hide resolved
@draganescu draganescu added Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. labels Dec 7, 2020
grzegorz_marzencki added 6 commits December 8, 2020 02:47
…d - 1. get default styling (black text) 2. diff the default styling with styles of pasted element to get all nondefault styles applied to element 3. Sanitize output 4. paste the output
@draganescu draganescu added [Package] Components /packages/components [Type] Enhancement A suggestion for improvement. labels Jan 8, 2021
@jameskoster
Copy link
Contributor

I'm not sure if this is caused by the overlay, or an issue with the buttons inherent disabled state (if so feel free to ignore), but I don't think the appearance should change on hover if the button is disabled.

@grzim
Copy link
Contributor Author

grzim commented Jan 13, 2021

Nice catch! I have checked it and this behaviour is not connected to an overlay itself.

@jameskoster
Copy link
Contributor

In that case I don't think this really needs a design review since nothing has changed visually.

@shaunandrews
Copy link
Contributor

In my opinion, this change would benefit UX.

Allowing disabled buttons to display a tooltip makes a lot of sense to me.

@shaunandrews shaunandrews removed the Needs Design Feedback Needs general design feedback. label Jan 13, 2021
@grzim grzim force-pushed the disabled-button-with-tooltip branch from 9a84643 to 589553f Compare January 14, 2021 07:09
@grzim grzim requested a review from ellatrix as a code owner January 14, 2021 07:09
@grzim grzim requested a review from talldan January 17, 2021 16:13
@grzim grzim requested a review from draganescu January 20, 2021 11:17
Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

Tested this and looks great. Code lgtm 🥇

@draganescu draganescu merged commit e4a6c1e into WordPress:master Jan 21, 2021
@github-actions github-actions bot added this to the Gutenberg 9.9 milestone Jan 21, 2021
@grzim grzim deleted the disabled-button-with-tooltip branch January 21, 2021 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Accessibility Feedback Need input from accessibility [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooltip component does not work with disabled Button component
4 participants