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

Popover style component which can be displayed on Hover #7469

Closed
achyutjhunjhunwala opened this issue Jan 22, 2024 · 7 comments
Closed

Popover style component which can be displayed on Hover #7469

achyutjhunjhunwala opened this issue Jan 22, 2024 · 7 comments

Comments

@achyutjhunjhunwala
Copy link

achyutjhunjhunwala commented Jan 22, 2024

Summary

We have quite some requirements where we want to use a popover on a hover action rather than onClick.
I understand from the EUI guidelines that Tooltip should be used for hover actions and Popover for click.

But sometimes we have situations where a click action on the item is already triggering an action and we cannot allow click for popover. For example, the Datatable header, we want an icon to appear on the header which should onHover render the popover but the datatable header already has an onClick action.

Why we cannot use a tooltip in that case ?

Because the tooltip limits us around customisation of the content we want to render.

Considerations

@1Copenut Highlighted some very good points, listing them here

  1. I've been seeing a mixture of click and hover || focus tooltips and popovers lately that both use icons as their triggering element. This feels confusing from a UX perspective. I have an opinionated expectation that any icons like the ? are hover/focus tooltips and bigger popovers with more information and focusable elements should be something else visually.
  2. An element that triggers on click AND hover || focus is going to cause friction. That's a hot take, but not a completely unfounded one. EuiPopover is wired to listen for the ESC key to close and return focus to its trigger. EuiTooltip does not have this because it's a mousein || focus in and mouseout || blur out behavior. Conflating the two could exacerbate a situation I've been seeing where we have popovers in modals and I press ESC thinking it'll close the popover and it closes the popover and the modal instead.
  3. The screenshots are very helpful to understand the use case. The information density is high in the third screenshot where theres' an ...Actions menu at the bottom. It may be a UX item where we assign a different visual to these more dense popovers as a cue that they're different than tooltips with different handling.

Screenshot of Use Case

Figma Link

image
@tkajtoch
Copy link
Member

tkajtoch commented Mar 4, 2024

I've reread our discussions on Slack and here and wanted to post an answer here as well. Adding a hover trigger to EuiPopover isn't something we like to add as an official option due to accessibility concerns. It's technically possible to achieve this effect programmatically by controlling the isOpen prop, but EuiPopover won't handle accessibility features for you (like detecting keypresses) when you do that, so we can't recommend that.

We are exploring the other solution to your problem which is modifying EuiDataGrid's column header to not open its context menu on header click but only when the dots icon button is focused. This way, you could hopefully achieve similar results while keeping it accessible. We'll keep you posted!

@cee-chen
Copy link
Member

cee-chen commented Mar 4, 2024

CC @MichaelMarcialis - any design suggestions here?

@MichaelMarcialis
Copy link
Contributor

CC @MichaelMarcialis - any design suggestions here?

@cee-chen: Regarding the notion of a hover action triggering the appearance of a popover? Or regarding @tkajtoch's proposal for the EuiDataGrid headers?

Speaking personally, I'd like to avoid supporting hover actions for popovers. It muddies the waters too much between the tooltip and popover components. Beyond the accessibility concerns that @1Copenut already provided, I'd prefer we keep hover events for tooltips and click events for popovers to ensure consistency/predictability for users across Kibana and to prevent scenarios where we are placing actionable content in hover-based elements (creating a potentially frustrating user experience).

@achyutjhunjhunwala: You stated above that the current tooltip component limits content customization. Would you mind elaborating on that? Assuming the content that it is meant to house isn't actionable, why couldn't we house the "Resources", "Content", and "Actions" popover contents from your screenshot in the existing tooltip component? The tooltip's content prop accepts a ReactNode.

As for @tkajtoch's proposal for changing the click to be only on the icon button rather than the whole header, I think that makes sense and would be a good change.

@tkajtoch
Copy link
Member

I'm going to create an issue to update EuiDataGrid column header to trigger opening the context menu on icon click. Thanks y'all for your feedback!

@tkajtoch
Copy link
Member

tkajtoch commented Apr 8, 2024

I've created #7660 to cover the work required for this effort and assigned it a medium priority. Let me know if this should be a higher priority!

@tkajtoch tkajtoch removed their assignment Apr 8, 2024
@tkajtoch
Copy link
Member

tkajtoch commented Apr 8, 2024

We're closing this issue since it no longer covers what needs to be done. Please refer to the newly created issue linked above for more details.

@tkajtoch tkajtoch closed this as completed Apr 8, 2024
@cee-chen cee-chen closed this as not planned Won't fix, can't repro, duplicate, stale Apr 8, 2024
@achyutjhunjhunwala
Copy link
Author

achyutjhunjhunwala commented Apr 30, 2024

@MichaelMarcialis I was on paternity leave and hence couldn't reply earkier. I guess i missed the fact that Tooltip takes ReactNode also as a prop. But i really like the idea @tkajtoch suggested. That should solve the problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants