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

EuiSuperDatePicker should show tooltip for the popover close event #3127

Merged
merged 13 commits into from
Mar 23, 2020

Conversation

ashikmeerankutty
Copy link
Contributor

@ashikmeerankutty ashikmeerankutty commented Mar 19, 2020

Summary

Fixes #3124; Fixes #1708

Added isPopoverClosed prop to EuiSuperUpdateButton component that will return true if both the start and end popovers are closed.
If there is a change in the value and the popover is closed the tooltip is displayed.
The showToolTip function is not called for all update operations and is called only if isPopoverClosed

Before

before1

After

after1

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
    - [ ] Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cchaos
Copy link
Contributor

cchaos commented Mar 19, 2020

@ashikmeerankutty Please describe in the summary how you have fixed the issue.

@ashikmeerankutty
Copy link
Contributor Author

@cchaos Thanks for the review, updated the description with changes. Can you please review it.

@chandlerprall chandlerprall self-requested a review March 19, 2020 21:16
@chandlerprall
Copy link
Contributor

This is definitely a lot better than being overwhelmed by the tooltip. Two things that could make this even better, if we can get the popover stuff to function this way:

update prevents interaction

  • the tooltip no longer shows up if hovering over the Update button while the selection panel is open - would be great to continue showing the tooltip whenever the cursor is over Refresh
  • have to click on the Update button twice (if the panel is open) to trigger an update - less than ideal, should only have to click once

@ashikmeerankutty
Copy link
Contributor Author

@chandlerprall Fixed both issues. Can you please review it now?

@cchaos
Copy link
Contributor

cchaos commented Mar 20, 2020

Jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3127/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; pulled & tested in the docs

@chandlerprall
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3127/

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM functionally. Just have one change request for the prop description.

@cchaos cchaos merged commit 0a201b2 into elastic:master Mar 23, 2020
@ashikmeerankutty ashikmeerankutty deleted the fix3/super-date-picker branch April 15, 2020 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants