-
Notifications
You must be signed in to change notification settings - Fork 839
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
[EuiToolTip] Enforce only one visible tooltip at a time #6520
Merged
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
38c0416
[misc cleanup] Group relative imports by general concept
cee-chen 6a9d296
Add tooltip manager that hides all other tooltips when a new tooltip …
cee-chen 36f0421
Write Cypress E2E tests for multiple tooltip behavior
cee-chen c73f41c
Changelog
cee-chen 8fabdb3
spellingz
cee-chen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
/// <reference types="../../../cypress/support"/> | ||
|
||
import React from 'react'; | ||
|
||
import { EuiButton } from '../../components'; | ||
import { EuiToolTip } from './tool_tip'; | ||
|
||
describe('EuiToolTip', () => { | ||
it('shows the tooltip on hover', () => { | ||
cy.mount( | ||
<EuiToolTip content="Tooltip text here" data-test-subj="tooltip"> | ||
<EuiButton data-test-subj="toggleToolTip">Show tooltip</EuiButton> | ||
</EuiToolTip> | ||
); | ||
cy.get('[data-test-subj="tooltip"]').should('not.exist'); | ||
cy.get('[data-test-subj="toggleToolTip"]').trigger('mouseover'); | ||
cy.get('[data-test-subj="tooltip"]').should('exist'); | ||
}); | ||
|
||
it('shows the tooltip on keyboard focus', () => { | ||
cy.mount( | ||
<EuiToolTip content="Tooltip text here" data-test-subj="tooltip"> | ||
<EuiButton data-test-subj="toggleToolTip">Show tooltip</EuiButton> | ||
</EuiToolTip> | ||
); | ||
cy.get('[data-test-subj="tooltip"]').should('not.exist'); | ||
cy.get('[data-test-subj="toggleToolTip"]').focus(); | ||
cy.get('[data-test-subj="tooltip"]').should('exist'); | ||
}); | ||
|
||
it('does not show multiple tooltips if one tooltip toggle is focused and another tooltip toggle is hovered', () => { | ||
cy.mount( | ||
<> | ||
<EuiToolTip content="Tooltip A" data-test-subj="tooltipA"> | ||
<EuiButton data-test-subj="toggleToolTipA">Show tooltip A</EuiButton> | ||
</EuiToolTip> | ||
<EuiToolTip content="Tooltip B" data-test-subj="tooltipB"> | ||
<EuiButton data-test-subj="toggleToolTipB">Show tooltip B</EuiButton> | ||
</EuiToolTip> | ||
</> | ||
); | ||
cy.get('[data-test-subj="tooltip"]').should('not.exist'); | ||
|
||
cy.get('[data-test-subj="toggleToolTipA"]').focus(); | ||
cy.contains('Tooltip A').should('exist'); | ||
cy.contains('Tooltip B').should('not.exist'); | ||
|
||
cy.get('[data-test-subj="toggleToolTipB"]').trigger('mouseover'); | ||
cy.contains('Tooltip B').should('exist'); | ||
cy.contains('Tooltip A').should('not.exist'); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import { toolTipManager } from './tool_tip_manager'; | ||
|
||
describe('ToolTipManager', () => { | ||
describe('registerToolTip', () => { | ||
const hideToolTip = jest.fn(); | ||
|
||
it('stores the passed hideToolTip callback', () => { | ||
toolTipManager.registerTooltip(hideToolTip); | ||
|
||
expect(toolTipManager.toolTipsToHide.has(hideToolTip)).toBeTruthy(); | ||
}); | ||
|
||
it('calls the previously stored hideToolTip callback and removes it from storage', () => { | ||
toolTipManager.registerTooltip(() => {}); | ||
|
||
expect(hideToolTip).toHaveBeenCalledTimes(1); | ||
expect(toolTipManager.toolTipsToHide.has(hideToolTip)).toBeFalsy(); | ||
}); | ||
}); | ||
|
||
describe('deregisterToolTip', () => { | ||
// If the current tooltip is already hidden before the next tooltip is visible, | ||
// there's no need to re-hide it, so we deregister the callback | ||
const deregisteredHide = jest.fn(); | ||
|
||
it('removes the hide callback from storage', () => { | ||
toolTipManager.registerTooltip(deregisteredHide); | ||
toolTipManager.deregisterToolTip(deregisteredHide); | ||
toolTipManager.registerTooltip(() => {}); | ||
|
||
expect(deregisteredHide).toHaveBeenCalledTimes(0); | ||
expect(toolTipManager.toolTipsToHide.has(deregisteredHide)).toBeFalsy(); | ||
}); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
/** | ||
* Manager utility that ensures only one tooltip is visible at a time | ||
* | ||
* UX rationale (primarily for mouse-only users): | ||
* @see https://github.com/elastic/kibana/issues/144482 | ||
* @see https://github.com/elastic/eui/issues/5883 | ||
*/ | ||
class ToolTipManager { | ||
// We use a set instead of a single var just in case | ||
// multiple tooltips are registered via async shenanigans | ||
toolTipsToHide = new Set<Function>(); | ||
|
||
registerTooltip = (hideCallback: Function) => { | ||
this.toolTipsToHide.forEach((hide) => hide()); | ||
this.toolTipsToHide.clear(); | ||
this.toolTipsToHide.add(hideCallback); | ||
}; | ||
|
||
deregisterToolTip = (hideCallback: Function) => { | ||
this.toolTipsToHide.delete(hideCallback); | ||
}; | ||
} | ||
|
||
export const toolTipManager = new ToolTipManager(); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
**Breaking changes** | ||
|
||
- `EuiToolTip`s now internally enforce only showing **one** tooltip at a time (the most recently triggered tooltip). This primary affects scenarios where users are focused on a tooltip toggle via click, and then hover onto another tooltip toggle. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I labelled this as a breaking change primarily due to the UX change, but I'm super open to thoughts if folks feel otherwise. I'm also not very sold on this changelog and happy to take feedback if it's overly verbose/wordy. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge kudos to Chandler for suggesting this singleton architecture in #6423 (comment) (vs creating a context and having context manage state) - this ended up being much simpler than I thought it would be!