The Wayback Machine - https://web.archive.org/web/20201108170621/https://github.com/ng-bootstrap/ng-bootstrap/pull/3794
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

Sticky popovers and tooltips #3794

Open
wants to merge 9 commits into
base: master
from

Conversation

@earshinov
Copy link

@earshinov earshinov commented Jun 29, 2020

Before submitting a pull request, please make sure you have at least performed the following:

  • read and followed the CONTRIBUTING.md and DEVELOPER.md guide.
  • built and tested the changes locally.
  • added/updated any applicable tests.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.

This PR attempts to solve the problem described in #2169,

Motivation

In our project we have popovers with information that users might like to copy, and also popovers with links for users to click. I consider these scenarios fairly basic and common, but ngbPopover doesn't really support them because it closes as soon as the user moves the mouse out of the element the popover is bound to (I will call it the target element from here on).

Here are some "solutions" we considered:

  • Force our users to make a click to open the popup
  • Confugure [closeDelay] large enough for users to be able to click a link before the popup closes
  • Use code snippets from #2169.

None of these are acceptable.

Proposal

The basic idea is to listen to trigger events both on the target element AND the popover element. To illustrate, in case of triggers="mouseenter:mouseleave" the popover will close on mouseleave UNLESS mouseenter was triggered when the user moved the cursor from the target element to the popover.

Of course, the cursor isn't moved instantaneously, so some delay is required. You can find it in the new NgbPopoverConfig.mouseleaveCloseDelay field and NgbPopover.mouseleaveCloseDelay input. I chose 100 ms as the default, but you are welcome to suggest any other positive number. I will tell you more, there is a similar field and a similar input for focusout as well.

You might argue that this approach is wrong and these events shouldn't be treated as special citizens. Maybe they shouldn't, but although I am open to suggestions regarding design and refactoring I would prefer to keep the code dumb and not over-complicate things until the basic idea is approved and my approach reviewed and tested. (Also I must note here that events will need to be handled differently anyway because they require different delays: mouse movement does not happen instantly while clicks and focus changes do.)

Among the enhancements themselves this PR includes:

  1. A couple of new unit tests which appropriately fail without the enhancements
  2. A couple of new buttons on the popover demo page so that the updated behavior can be seen in action.
  3. Similar enhancements, unit tests and demos for ngbTooltip

There are some technical details I would like to point out:

  1. In order to detect focus events, a popover needs to be focusable (have tabIndex)
  2. "Click" triggers are excluded from the general rule: in contrast to other trigger events, clicks are not listened to on the popover element. This is because clicks outside the target element are already nicely handled by autoClose.

Update: fixed some typos.

*
* @since *Unreleased*
*/
@Input() mouseleaveCloseDelay: number;

This comment has been minimized.

@earshinov

earshinov Jun 29, 2020
Author

Do I need to prepare API documentation other than these comments?

* The delay before the popover is closed on mouseleave. Only takes effect if "mouseleave" is among the close
* triggers.
*
* @since *Unreleased*

This comment has been minimized.

@earshinov

earshinov Jun 29, 2020
Author

*Unreleased* to be replaced with an appropriate version at the time of merge.

@benouat
Copy link
Member

@benouat benouat commented Jul 21, 2020

Hey @earshinov! Sorry to comment out this late on your PR.

Allow me to challenge you that what you try to fix in this PR is not needed in the first place. 😉
Please have a look a my #2169 (comment) in which I try to describe and explain the confusion around both popovers and tooltips.

My main concern was to try to give some answers somehow based on your description

In our project we have popovers with information that users might like to copy, and also popovers with links for users to click. I consider these scenarios fairly basic and common, but ngbPopover doesn't really support them because it closes as soon as the user moves the mouse out of the element the popover is bound to (I will call it the target element from here on).

I have gathered my solution in a StackBlitz available here
Could you please have a look at it, and see if it could suit your needs ? Thanks! 🙏

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.