cytoscape / cytoscape.js-qtip

A Cytoscape.js extension that wraps the QTip jQuery library
MIT License
42 stars 35 forks source link

NO-REF - Use DOM to position toolip to allow `flip` to work. #19

Closed nowells closed 7 years ago

nowells commented 8 years ago

qTip has great support for the viewport flip/shift adjustments that will switch the tooltip positions when approaching viewport boundaries. However, it requires DOM elements to do computation, and the manual adjustments that we currently apply do not play nicely with that tooling. To address this I took the approach of adding a temporary DOM element to provide to qTip (and then immediately remove it). We could do this, or simply add a PR to qTip to allow passing an object to position.target with {x: ele.x, y: ele.y, width: ele.width, height: ele.height} (which they do not support, they support [x, y] but it needs width to allow things like flip to work.

How do you feel about fixing this here vs upstream? I think there might be more advantages to using a proper DOM el for qtip like I do, but wanted to lay out my thinking.

Cheers, Nowell

maxkfranz commented 8 years ago

The problem with using a dummy dom element for positioning the qtip is that it can be slow for graphs with lots of elements.

There are a few usecases to consider:

(1) My graph uses only one visible qtip at a time. This is a common usecase. You might get away with re-using just one dummy dom element for this case, and moving it before each show.

(2) My graph uses multiple visible qtips at a time and I care more about the speed of the interface rather than the positioning. The current solution addresses this.

(3) My graph uses multiple visible qtips at a time and I care more about positioning than speed. Your solution addresses this.

Probably we need an option to set whether (1), (2), or (3) is used -- if qtip doesn't allow passing { x, y, w, h } like you suggest.

Ideally, qtip would support sending your bounding box object, because that's much faster. I don't have a problem having options for (1), (2), and (3) in the extension in the meantime. Your PR is for (3), so we could just have (2) and (3) for now.

I can review the PR itself on the 28th. (I'm away.)

maxkfranz commented 7 years ago

@nowells I played around with your changes a bit.

It seems to work well for all cases now. Released as 2.5.0. Thanks!