Drawing bbox fix for window scroll #1683
Conversation
| left: -20.671875, | ||
| top: -11, | ||
| right: 20.34375, | ||
| bottom: 3 |
There was a problem hiding this comment.
Commit 29abb5d made the bottom and top keys here incorrectly change - as the bbox computation were based on a outdated cached tester element.
There was a problem hiding this comment.
The width, left and right keys change correctly here in response to the annotation text relayout update.
There was a problem hiding this comment.
Ha, those values appear to be machine-dependent (I suspect a font issue) ...
- so that Drawing.bBox does not have to query the DOM to fing them on every call.
- to not have to instantiate one on every appendSVG call
src/components/drawing/index.js
Outdated
| if(!drawing.refRect) { | ||
| drawing.refRect = drawing.test3.select('.js-reference-point') | ||
| var testRect = testNode.getBoundingClientRect(), | ||
| refRect = test3.select('.js-reference-point') | ||
| .node().getBoundingClientRect(); |
There was a problem hiding this comment.
If you want to keep the perf gains from #1585 in this fix, I'm pretty sure the only change we'd need is to call this .getBoundingClientRect() with each drawing.bBox call, rather than caching it. But we can still cache tester, and we can cache test3.select('.js-reference-point').node().
Just a question of timing really - now that you have a scrolling test in place it seems safe to me, but you can leave it as is for now if you need to move on to other things.
|
💃 once #1690 is merged and tests pass |
fixes #1678
I decide to play it safe an revert 29abb5d until we 🔒 the
Drawing.bBoxbehavior better. Reverting 29abb5d also fixes #1675, but maybe we could do better there.cc @alexcjohnson