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

[EuiDatePicker] Better handle onBlur callback #5441

Merged
merged 11 commits into from
Dec 6, 2021

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Dec 3, 2021

Summary

Addresses a specific bug whereby wrapping EuiDatePicker in EuiToolTip will cause the tooltip to never close once opened: https://codesandbox.io/s/nervous-smoke-kk5ys
The root problem was that provided onBlur callbacks are simply ignored, and as such the solution applies more broadly to all blur event scenarios.
The intended behavior is that onBlur callbacks only get called when the entire component loses focus. That is, if either the input or any element in the popover has focus, we treat that as internal focus transfer and not blur.

Additional update includes a change to EuiToolTip so that is does not omit configured onBlur and onFocus callbacks on the child component.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Added or updated jest and cypress 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
  • Reverted 70dc65b

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

I did a quick run through with Safari and Chrome, looking specifically at keyboard and click navigation patterns. They all worked well, and honored the blur() event to hide the tooltip.

@kibanamachine
Copy link

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

@thompsongl thompsongl marked this pull request as ready for review December 3, 2021 20:21
@kibanamachine
Copy link

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

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

I've taken a pass through with Firefox, Chrome, and Edge on MacOS locally to test for consistent behavior with the tooltip. All were behaving as expected. LGTM!

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

This looks good, Greg! I ran through the changes in the PR preview and compared it to the current version of the docs. The updated onBlur functionality creates a clean experience.

@kibanamachine
Copy link

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

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.

4 participants