[FEATURE] ScrollTo and ScrollToRow #228

buschtoens · 2016-10-18T00:11:26Z

Closes #148.

buschtoens · 2016-10-18T00:21:15Z

I pushed a build to my gh-pages for your convenience.

Are you happy with the way the demo and actual code are implemented or would you like me to make changes?

buschtoens · 2016-10-18T00:22:56Z

This PR deliberately ignores the issues raised in #224. I'll tackle them in one go when this is merged.

buschtoens · 2016-10-18T00:33:06Z

Something worth noting:

In order for the showRow feature to work, I needed a way to retrieve the element.offsetTop property of the corresponding {{lt-row}} component for a given Row instance.

Currently the {{lt-row}} component has a reference to the Row instance, but not the other way around. I therefore added the private rowComponent property to the Row class which is populated in the didInsertElement hook of the {{lt-row}} component.

Maybe we can even make rowComponent and scrollOffset public.

I feel like this is the cleanest way. But I'm up for discussion. An alternative would be getting the this.get('table.rows').indexOf(targetRow) and querying the DOM for the n-th row.

If everything's in the clear, I'll add the necessary tests in another commit and remove the WIP tag. 😄

buschtoens · 2016-10-18T00:52:31Z

Oh, and the "keep fetching rows until the targetScrollOffset is reached" feature isn't implemented yet.

For this I'd check if the targetScrollOffset is greater than then scrollOffset of the last Row in table.rows and in that case trigger onScrolledToBottom by attempting to scroll to that position nevertheless and keep on doing so until the previous condition is false. For keeping on doing that an observer on table.rows is required.

Or can you suggest an observer-free alternative?

offirgolan · 2016-10-20T07:56:30Z

@buschtoens thank you for taking the time in putting this together. I do have a few concerns.

Offset logic should not be placed in the Row class.
What happens when we are not using ember-scrollable (i.e. simple use cases with no fixed header/footer?
Why do we need to keep fetching rows until the target row is reached? If we require the Row instance to exist, shouldn't that instance already be in the table / rendered?

In regards to my first concern, I believe we can use the row instance's guid as the id of the lt-row component instance'd id. With that, we can get the offset of the row from the lt-body.

id: computed(function() {
  return guidFor(this);
}).readOnly()

{{lt-row id=row.id row=row ...}}

buschtoens · 2016-10-20T18:42:44Z

Thanks for replying!

Offset logic should not be placed in the Row class.

I agree with that. Using the GUID is a neat idea.

Why do we need to keep fetching rows until the target row is reached? If we require the Row instance to exist, shouldn't that instance already be in the table / rendered?

This is true when using the showRow interface. But there's also a scrollTo interface which just takes a numeric scroll offset in px. Primarily I'm interested in the latter one to easily restore state after changing the route.

What happens when we are not using ember-scrollable (i.e. simple use cases with no fixed header/footer?

So far, nothing. As far as I'm concerned I only need these features when I have a table that is intended to overflow and scroll, in which case I'm already opting for the fixed headers/footers.

I can't think of a legit reason to not use fixed headers/footers, other than possible performance gains, because the UX of disappearing headers/footers is pretty moot.

Options:

a. Explicitly document that showRow, scrollTo and onScroll only work when using fixed headers/footers.
b. Basically reimplement alphasights/ember-scrollable#25, however this only works when the user's CSS targets the {{lt-body}} as the scroll container.

offirgolan · 2016-10-20T22:54:35Z

This is true when using the showRow interface. But there's also a scrollTo interface which just takes a numeric scroll offset in px. Primarily I'm interested in the latter one to easily restore state after changing the route.

Ah right. I forgot about that use case.

Explicitly document that showRow, scrollTo and onScroll only work when using fixed headers/footers.

I think this is okay 😄

offirgolan · 2016-10-24T03:17:37Z

@buschtoens dont worry too much on the demo. When this is fully functional and merged, I'll go back and make some changes. I also have a client that will require this feature so if you need any help, just let me know or shoot me a message on slack.

buschtoens · 2016-10-24T07:44:45Z

@offirgolan Ready for review. :)

offirgolan

@buschtoens added some review comments. Let me know if you need more clarification on something 😄

offirgolan · 2016-10-24T08:05:04Z

addon/classes/Row.js

+   * @type {String}
+   * @readOnly
+   */
+  elementId: computed(function() {


Lets keep this as id and just return the guid. This should be unique enough. I just dont think any property here should directly correlate to a component.

Note: The Column class will also have the same implementation once I finish the drag n drop column headers feature.

I named this field elementId, so it does not shadow the id field of the content object which is most likely an Ember Data Record.

Still want me to rename it?

Ah thats a good call. Maybe change this to rowId and add a small comment as to why we didn't go by id (just so I randomly change this later 😛 )

offirgolan · 2016-10-24T08:06:04Z

addon/components/lt-body.js

+   * @type {Row}
+   * @default null
+   */
+  showRow: null,


I think scrollToRow is a more fitting name here and is consistent with the other scrollTo property.

offirgolan · 2016-10-24T08:09:07Z

addon/components/lt-body.js

@@ -239,6 +292,57 @@ export default Component.extend({
    this.set('useVirtualScrollbar', fixedHeader || fixedFooter);
  },

+  didReceiveAttrs() {


Would it be better to handle this with didUpdateAttrs since it gives you the before and after values? That could remove the need of having _scrollTo and _scrollToRow. You can handle the initial scroll with didInsertElement

I didn't know that didUpdateAttrs() actually can be used to diff the attrs. Nice one, thanks!

offirgolan · 2016-10-24T08:10:15Z

addon/components/lt-body.js

+    if (scrollTo !== _scrollTo) {
+      this.set('targetScrollOffset', scrollTo);
+      this.set('hasReachedTargetScrollOffset', false);
+      run.scheduleOnce('afterRender', this, this.checkTargetScrollOffset);


I noticed this is done in more than one place. Lets move this to another method.

Also, please make sure to capture all timer instances and cancel them in destroy.

Also, none of this should be happening if scrollTo and scrollToRow are not set.

Side note: checkTargetScrollOffset() is only applicable for scrollTo. Other than that I agree.

offirgolan · 2016-10-24T08:12:08Z

addon/components/lt-body.js

+   * @default null
+   * @private
+   */
+  targetScrollOffset: null,


Set the default here to 0

This would trigger a scroll to 0 px down the line. However that shouldn't matter as we start at 0 px anyway. So for clarity's sake, you're probably right.

offirgolan · 2016-10-24T08:12:13Z

addon/components/lt-body.js

+   * @default null
+   * @private
+   */
+  currentScrollOffset: true,


Set the default here to 0

Yeah, that was a copy pasta typo.

offirgolan · 2016-10-24T08:13:36Z

addon/components/lt-body.js

+   * @default true
+   * @private
+   */
+  hasReachedTargetScrollOffset: true,


Can this be a CP? Something like:

hasReachedTargetScrollOffset: computed.gte('currentScrollOffset', 'targetScrollOffset');

This cannot be a CP. The check you mentioned is performed inside the checkTargetScrollOffset() method.

This property stores state. checkTargetScrollOffset() is called every time new rows are added. But it's only supposed to keep scrolling until the desired offset has been reached once.

offirgolan · 2016-10-24T08:17:16Z

addon/templates/components/lt-body.hbs

@@ -18,6 +20,7 @@
        {{else}}
          {{#each rows as |row|}}
            {{lt.row row columns
+              elementId=row.elementId


Actually has to be named elementId.

Ah gotcha. Thanks for clarifying. I believe the id field is an alias to the elementId

buschtoens · 2016-10-24T10:59:31Z

addon/components/lt-body.js

+    let newScrollTo = get(newAttrs, 'scrollTo.value');
+    let oldScrollTo = get(oldAttrs, 'scrollTo.value');
+    let newScrollToRow = get(newAttrs, 'scrollToRow.value');
+    let oldScrollToRow = get(oldAttrs, 'scrollToRow.value');


The oldAttrs and newAttrs hashes are not documented and appear to be foreign Glimmer objects. I'm not sure if this is the correct way to access the values.

Huh... you're totally right. That functionality is no longer documented. Lets revert back to your previous implementation.

So instead of using didUpdateAttrs we should go back to didReceiveAttrs and store private copies of the attributes for diffing?

Yeah. I spoke with locks and confirmed that we should not be using the arguments in didUpdateAttrs. Sorry for making you revert 😅

buschtoens · 2016-10-24T11:07:40Z

addon/components/lt-body.js

+   * @default 0
+   * @private
+   */
+  targetScrollOffset: 0,


Due to this property triggering an initial scroll, this PR is blocked until alphasights/ember-scrollable#26 gets published.

buschtoens · 2016-10-24T11:14:02Z

addon/components/lt-body.js

+  }),
+
+  checkTargetScrollOffset() {
+    if (!this.get('hasReachedTargetScrollOffset') && !this.get('isDestroyed')) {


I'm checking for isDestroyed instead of cancelling the queued task, which effectively achieves the same thing. Are you okay with this?

I dont think this is enough. On line 334 you are doing a set on what could be a destroyed object. I think capturing all viable timers and cancelling them on destroy is a much safer bet.

offirgolan · 2016-10-24T11:31:14Z

addon/components/lt-body.js

+  },
+
+  rowObserver: observer('rows.[]', function() {
+    run.scheduleOnce('afterRender', this, this.checkTargetScrollOffset);


@buschtoens I suggest adding a private method:

_scheduleCheckTargetScrollOffset() { run.cancel(this._checkTargetOffsetTimer); // Cancel existing checks since only one needs to happen (I'm not sure if this is actually needed since we are using scheduleOnce.... but just to be safe? this._checkTargetOffsetTimer = run.scheduleOnce('afterRender', this, this.checkTargetScrollOffset); } destroy() { this._super(...arguments); run.cancel(this._checkTargetOffsetTimer); }

This wouldn't account for the timer in line 334. But tbh, I doubt that the timer in 334 could ever cause problems.

run.cancel(...) inside _scheduleCheckTargetScrollOffset() is unnecessary.

Relevant code in Backburner:
https://github.com/ebryn/backburner.js/blob/master/lib/backburner/deferred-action-queues.js#L38-L42
https://github.com/ebryn/backburner.js/blob/master/lib/backburner/queue.js#L33-L37

I doubt that the timer in 334 could ever cause problems.

I've run into a couple issues like that in the past. Better to be cautious I suppose 😜

run.cancel(...) inside _scheduleCheckTargetScrollOffset() is unnecessary.

Even better.

So should I add a second reference for that timer as well?

Edit: Did so. 😄

offirgolan · 2016-10-24T12:00:40Z

addon/components/lt-body.js

+      if (newScrollTo > 0) {
+        hasReachedTargetScrollOffset = false;
+      }
+    } else if (oldScrollToRow !== newScrollToRow) {


Can this just be else if (newScrollToRow instanceof Row && oldScrollToRow !== newScrollToRow)?

Another option is to have an assert in didReceiveAttrs

buschtoens · 2016-10-24T12:58:26Z

Okay, so I reverted back to using didReceiveAttrs and manual diffing. If I'm not mistaken, that should be all. 😆

Then there's only alphasights/ember-scrollable#26 left :)

* master: [FEATURE] ScrollTo and ScrollToRow (#228)

buschtoens · 2016-10-25T18:18:29Z

Sorry for the silence from my side. I was a bit under the weather over the
course of the last days. I'll finish the PR in a few hours when I'm in the
office. :)

We're also eagerly waiting for this feature :D

On Mon, Oct 24, 2016, 05:17 Offir Golan notifications@github.com wrote:

@buschtoens https://github.com/buschtoens dont worry too much on the
demo. When this is fully functional and merged, I'll go back and make some
changes. I also have a client that will require this feature so if you need
any help, just let me know or shoot me a message on slack.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#228 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAy8TGxRWrSTqoV4a-y5QrXzck-wONeLks5q3CNRgaJpZM4KZPhL
.

* master: (42 commits) Resizable column fixes (#252) fix: repeated scrollToRow (#254) chore(package): update ember-cli-code-coverage to version 0.3.7 (#253) Demo Cleanup (#251) chore(package): update ember-lodash to version 0.0.11 (#245) chore(package): update ember-cli-code-coverage to version 0.3.6 (#249) Document `Row#content` (#250) Update resizable snippet Released v1.5.2 [FEATURE] minResizeWidth + Event bubbling fix (#244) Update readme Update readme Released v1.5.1 Add safe checks scroll logic (#241) [DOCFIX] Make breakpoints of demo app the same as ember-responsive's default (#240) Released v1.5.0 Demo + Scrolling Cleanup (#238) [FEATURE] Responsive Columns (#235) [FEATURE] ScrollTo and ScrollToRow (#228) chore(package): update broccoli-asset-rev to version 2.5.0 (#227) ...

buschtoens force-pushed the 148-scroll-control branch from a33dbe4 to e9275c5 Compare October 18, 2016 00:13

buschtoens force-pushed the 148-scroll-control branch 2 times, most recently from 787541f to 7e0b67c Compare October 18, 2016 00:50

buschtoens force-pushed the 148-scroll-control branch from 7e0b67c to 61e4a18 Compare October 18, 2016 14:36

buschtoens force-pushed the 148-scroll-control branch from e9847ae to bf76e7d Compare October 24, 2016 06:32

buschtoens changed the title ~~WIP: scroll control~~ scroll control Oct 24, 2016

offirgolan requested changes Oct 24, 2016

View reviewed changes

buschtoens added 7 commits October 24, 2016 11:56

examples: add example for scrolling API

04664fd

scroll control

99c1e6a

use element IDs for finding the target scroll offset

b0bd3a1

docs: scroll control only works when useVirtualScrollbar is true

e845089

keep fetching rows until the targetScrollOffset is reached

4c4ae2f

rename Row#elementId to rowId

8479169

rename lt-body#showRow to scrollToRow and use didUpdateAttrs

a6c8ece

buschtoens force-pushed the 148-scroll-control branch from 1ed7083 to 1b11b62 Compare October 24, 2016 10:56

buschtoens added 2 commits October 24, 2016 13:03

fix linter errors

38b97e8

set default values to 0 for scroll offsets

8c760ca

buschtoens force-pushed the 148-scroll-control branch from 3be68db to 38b97e8 Compare October 24, 2016 11:04

buschtoens commented Oct 24, 2016

View reviewed changes

offirgolan reviewed Oct 24, 2016

View reviewed changes

buschtoens added 2 commits October 24, 2016 13:44

cancel timer instead of checking isDestroyed

60b7f7c

also cancel inner targetScrollOffset timer

f982f08

offirgolan reviewed Oct 24, 2016

View reviewed changes

buschtoens added 2 commits October 24, 2016 14:42

clean up scroll setters

92bba17

revert to didReceiveAttrs and manual diffing

4b84f1a

buschtoens force-pushed the 148-scroll-control branch from 170ce20 to 4b84f1a Compare October 24, 2016 12:55

offirgolan approved these changes Oct 24, 2016

View reviewed changes

offirgolan changed the title ~~scroll control~~ [FEATURE] ScrollTo and ScrollToRow Oct 24, 2016

offirgolan merged commit 9afc7a9 into adopted-ember-addons:master Oct 25, 2016

offirgolan pushed a commit that referenced this pull request Oct 25, 2016

Merge branch 'master' into responsive

a7f4003

* master: [FEATURE] ScrollTo and ScrollToRow (#228)

[FEATURE] ScrollTo and ScrollToRow #228

[FEATURE] ScrollTo and ScrollToRow #228

Conversation

buschtoens commented Oct 18, 2016

buschtoens commented Oct 18, 2016

buschtoens commented Oct 18, 2016

buschtoens commented Oct 18, 2016 • edited Loading

buschtoens commented Oct 18, 2016 • edited Loading

offirgolan commented Oct 20, 2016

buschtoens commented Oct 20, 2016

offirgolan commented Oct 20, 2016

offirgolan commented Oct 24, 2016

buschtoens commented Oct 24, 2016

offirgolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

offirgolan Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

offirgolan Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buschtoens Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buschtoens commented Oct 24, 2016 • edited Loading

buschtoens commented Oct 25, 2016

buschtoens commented Oct 18, 2016 •

edited

Loading

buschtoens commented Oct 18, 2016 •

edited

Loading

offirgolan Oct 24, 2016 •

edited

Loading

offirgolan Oct 24, 2016 •

edited

Loading

buschtoens Oct 24, 2016 •

edited

Loading

buschtoens commented Oct 24, 2016 •

edited

Loading