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 dynamic start with offset #84

Merged
merged 25 commits into from
May 10, 2017
Merged

Fix dynamic start with offset #84

merged 25 commits into from
May 10, 2017

Conversation

runspired
Copy link
Contributor

@runspired runspired commented May 8, 2017

This adds a (was failing) test for starting a dynamically sized list in a position offset from the top of the scroll-container.

However, this causes the skip-list to need to deal with negative numbers. To handle this, I then changed the determination of middleVisibleValue to:

let top = this.visibleTop;

if (top < 0) {
  top = 0;
}

const middleVisibleValue = top + (this.scrollContainerHeight / 2);

@pzuraq
Copy link
Contributor

pzuraq commented May 8, 2017

Yeah, we should keep visibleTop here. scrollTopOffset is still needed, and prependOffset is whatever the offset we get when we do a prepend (in a prepend, we don't actually set scrollTop until after we've rendered to avoid setting it more than once, so we need to make our calculations with that in mind).

The correct behavior here is if visibleTop <= 0, we would render the first n elements (where n is the number of VCs). visibleTop is supposed to give us the ability to have a "negative scrollTop", which is useful for calculations and tracking state but makes no sense here. So it should probably be:

const middleVisibleValue = Math.max(this.visibleTop, 0) + (this.scrollContainerHeight / 2);

@runspired runspired force-pushed the fix-dynamic-start-with-offset branch from 2a5e8c3 to d88cba2 Compare May 8, 2017 15:01
Copy link
Contributor

@pzuraq pzuraq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in tests pass

@pzuraq pzuraq force-pushed the fix-dynamic-start-with-offset branch from 1317002 to 24fd753 Compare May 8, 2017 20:33
@pzuraq pzuraq force-pushed the fix-dynamic-start-with-offset branch from ea42d06 to 4264340 Compare May 9, 2017 23:03
@pzuraq pzuraq force-pushed the fix-dynamic-start-with-offset branch from 4264340 to 4d03c1d Compare May 9, 2017 23:04
@pzuraq pzuraq force-pushed the fix-dynamic-start-with-offset branch from edf67d6 to 159e991 Compare May 10, 2017 18:58
@pzuraq pzuraq merged commit ae05c63 into master May 10, 2017
@pzuraq pzuraq deleted the fix-dynamic-start-with-offset branch May 10, 2017 19:18
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.

None yet

2 participants