preventOverflow flag to offset tooltips to stay within viewport#953
preventOverflow flag to offset tooltips to stay within viewport#953edemaine wants to merge 2 commits intoseiyria:masterfrom
Conversation
|
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. |
|
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); |
There was a problem hiding this comment.
could you make sure this event listener is being removed in the _removeSliderEventHandlers cleanup method?
|
is there anyway to possibly add an automated test for this? i know sometimes it can be tricky with these types of changes |
|
Popper supports virtual reference elements, it would allow you to position a tooltip even if there's not a html element to attach to. |
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.
createPopperrequires 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 tocreatePopper, but I instead tried a fourth approach:preventOverflowmanually.This achieves the goal of no dependencies, adding a fairly small amount of code for nice functionality, hidden within a new
preventOverflowoption. 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 screenshotsI should mention that the logic I implemented is much simpler than Popper's
preventOverflow.jswhich seems to be mainly dealing with the case of clipping to the nearestdisplay: fixedancestor. I opted to just clip towindow. 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:
grunt testin your Terminal within the bootstrap-slider repository directory