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

preventOverflow flag to offset tooltips to stay within viewport #953

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

edemaine
Copy link
Contributor

@edemaine edemaine commented Jan 22, 2021

This PR fixes #649 and in some sense fixes #952.

I initially tried the second or third approach outlined in #952, but realized that it would be difficult to actually use Popper.js. createPopper requires a DOM element to anchor the tooltip to, and in some uses of bootstrap-slider, there isn't a tick associated with the position being tooltipped. We could create an artificial tooltip position DOM element to give to createPopper, but I instead tried a fourth approach:

  1. Implement the logic of preventOverflow manually.

This achieves the goal of no dependencies, adding a fairly small amount of code for nice functionality, hidden within a new preventOverflow option. I didn't change the defaults because it would break a lot of tests and backward compatibility, but happy to change.

I did not implement flip yet, mainly because I care about it less. I could work on it if desired.

I've gotten this working well in my project (via npm link), which uses left-to-right horizontal sliders and always-shown tooltips. Here are some screenshots

context screenshot
left extreme image
right extreme (including scrollbar) image
in the middle image

I should mention that the logic I implemented is much simpler than Popper's preventOverflow.js which seems to be mainly dealing with the case of clipping to the nearest display: fixed ancestor. I opted to just clip to window. There may be some subtleties I'm missing.

As this is my first contribution, I could use some guidance on how to test more thoroughly/broadly, and these PR requests:

  • unit tests (we use Jasmine 2.x.x)
  • JSFiddle (or an equivalent such as CodePen, Plunker, etc) or screenshot/GIF with new feature or bug-fix
  • Link to original Github issue (if this is a bug-fix)
  • documentation updates to README file
  • examples within /tpl/index.tpl (for new options being added)
  • Passes CI-server checks (runs automated tests, JS source-code linting, etc..). To run these on your local machine, type grunt test in your Terminal within the bootstrap-slider repository directory

@seiyria
Copy link
Owner

seiyria commented Jan 22, 2021

Thank you for this! It's the first contribution we've gotten in a while so please be patient. @rovolution - what do you think? You spent more time on this so I want to get your opinion as well if possible.

@edemaine
Copy link
Contributor Author

Thanks for all the quick responses! Also, if you'd rather run with this yourself, tweaking it how you'd like/rather it be, feel free to do so (e.g. pushing to this branch).

// Simulate Popper's behavior of listening to both resize and scroll:
// https://popper.js.org/docs/v2/modifiers/event-listeners/
if (this.options.preventOverflow) {
window.addEventListener("scroll", this.resize, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you make sure this event listener is being removed in the _removeSliderEventHandlers cleanup method?

@rovolution
Copy link
Collaborator

is there anyway to possibly add an automated test for this? i know sometimes it can be tricky with these types of changes

@FezVrasta
Copy link

FezVrasta commented Feb 18, 2021

Popper supports virtual reference elements, it would allow you to position a tooltip even if there's not a html element to attach to.

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.

Tooltips via Popper.js? tooltip is offset, out of the parent div
4 participants