Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(occlusion): pass bufferSize based on scrollBuffer #514

Merged
merged 3 commits into from
Nov 24, 2017

Conversation

buschtoens
Copy link
Collaborator

Closes #485.

@buschtoens
Copy link
Collaborator Author

Looking at the actions made available by vertical-collection, this PR (in it's current form) doesn't really gain us anything yet. While the bufferSize is correctly calculated, the onScrolledToBottom action is not called yet.

Therefore I'll keep this WIP and also try to solve #508 in this PR.

@buschtoens buschtoens changed the title fix(occlusion): pass bufferSize based on scrollBuffer [WIP] fix(occlusion): pass bufferSize based on scrollBuffer Nov 23, 2017
@buschtoens
Copy link
Collaborator Author

buschtoens commented Nov 23, 2017

Since we want to trigger onScrolledToBottom with scrollBuffer in mind, we should probably use lastVisibleChanged instead of lastReached.
Scratch that, turns out lastReached already accounts for bufferSize. 😊

onScroll could be shimmed with firstVisibleChanged. However the onScroll action wouldn't be called for every scroll event, but I think that's a worthwhile trade-off.

@buschtoens
Copy link
Collaborator Author

buschtoens commented Nov 23, 2017

Okay, so I got the actions wired up, but we have a problem.

vertical-collection is not happy about the {{#if isLoading}} construct... Essentially Glimmer needs a static element to render the contents of {{#if isLoading}} into. If you wrap it with a plain <div>, everything works fine, but of course that bricks the layout...

There is a private {{#-in-element someElement}} helper that takes the block and renders it into someElement. This would fix our problem, however we need something like {{#-after-element someElement}}, which doesn't exist yet.

Semi-related issue: yapplabs/ember-tether#35 (comment)

@buschtoens
Copy link
Collaborator Author

buschtoens commented Nov 24, 2017

While I'm trying to fix this (or get people to fix it 😜), we can still ship out a new version, since occlusion has to be enabled with a flag and tables with occlusion enabled and infinite scroll were essentially broken anyway. 😄

I'll drop a line in the release notes that instruct users to not use any conditionals in the the yielded block.

I'll just remove the {{yield}} block for now and add a line in the release notes about it. Seems safer and less troublesome for users.

Stuff like expanded-row also breaks v-c, since it has a top-level {{#if}} statement in its template.

buschtoens added a commit that referenced this pull request Nov 24, 2017
@buschtoens buschtoens changed the title [WIP] fix(occlusion): pass bufferSize based on scrollBuffer fix(occlusion): pass bufferSize based on scrollBuffer Nov 24, 2017
@buschtoens buschtoens merged commit 30ae851 into master Nov 24, 2017
buschtoens added a commit that referenced this pull request Nov 24, 2017
@buschtoens buschtoens deleted the 485-fix-occlusion-scroll-buffer branch November 24, 2017 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant