Improve footer tracking for bottom scroll indicator #873
Conversation
…leod.temp/improve-bottom-scroll-indicator-tracking
ahmacleod
force-pushed
the
alex.macleod.temp/improve-bottom-scroll-indicator-tracking
branch
from
97e11b5 to
5e9f9df
Compare
ahmacleod
force-pushed
the
alex.macleod.temp/improve-bottom-scroll-indicator-tracking
branch
from
5e9f9df to
ffa0efa
Compare
| @@ -269,6 +317,12 @@ export default Component.extend({ | |||
|
|
|||
| didInsertElement() { | |||
| this._super(...arguments); | |||
|
|
|||
| // debounced, runloop-safe version | |||
| this._updateIndicators = bind(this, () => { | |||
There was a problem hiding this comment.
I think that's a leftover, let's nix it.
There was a problem hiding this comment.
Aha, I forgot this is why:
You need to wrap the scheduleOnce in a run loop or else the test suite gets mad. You can't get at it from the test itself.
There was a problem hiding this comment.
Two more questions on this topic:
- Why do we set
this._updateIndicatorsinsidedidInsertElementvs.init? - Are there any issues with using an anonymous function vs. a function defined on the component?
There was a problem hiding this comment.
-
No objection; consider it moved.
-
Not afaik. Anything we move to a component function is ultimately going to have to be wrapped in a
bindor something that produces an anonymous function anyway. This forward declaration does a lot of heavy lifting:
- Context binding for a scroll listener, two
ResizeSensors, and aMutationObserver - Deals with the test runloop situation
- Encapsulates the
scheduleOnce - Simplifies removing the scroll listener on teardown; don't have to cache a
this._scrollListenerfunction
What we can do is dump the bind for a run. The context is already available via the anonymous closure, so run is a more precise solution to its problem and can be documented as such.
There was a problem hiding this comment.
run won't cut it for ember-2.8-lts test suite. I've notated the seemingly redundant bind for now; we can visit when we drop 2.8 support.
This reverts commit e2da2e9.
| @@ -269,6 +317,12 @@ export default Component.extend({ | |||
|
|
|||
| didInsertElement() { | |||
| this._super(...arguments); | |||
|
|
|||
| // debounced, runloop-safe version | |||
| this._updateIndicators = bind(this, () => { | |||
There was a problem hiding this comment.
Two more questions on this topic:
- Why do we set
this._updateIndicatorsinsidedidInsertElementvs.init? - Are there any issues with using an anonymous function vs. a function defined on the component?
ahmacleod
deleted the
alex.macleod.temp/improve-bottom-scroll-indicator-tracking
branch
In order to position the bottom scroll indicator correctly, we need to know if and when the visible portion of the footer changes height. Presently, we detect when the dimensions of the table's container changes, but not specifically when the height of the footer, or the position of its sticky cells, change. These changes can occur without altering the overall height or width of the table, which can leave the bottom scroll indicator floating in the wrong position.
So far this has only been a problem in testing environments, where it manifests as an erratic race condition during initial render. It is exacerbated by increased functionality in the footer (e.g. child rows), because of increased jostling during layout. During a typical application pageload, there are sufficiently many layout changes that the effect is rarely seen.
Since it is possible to programmatically add and remove a footer from the table, we install and remove those listeners dynamically.
Changes:
ResizeSensoron footer to detect changes in total footer heightMutationObserveron footer to detect changes to thestyleattribute oftdelements; we are interested in whenbottomchanges value