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(virtual-repeat): handle array changes correctly #107

Merged
merged 3 commits into from
Apr 5, 2017

Conversation

AStoker
Copy link
Contributor

@AStoker AStoker commented Mar 31, 2017

Resolves: #106, #98, #89, #87, #60
Previous behavior did not handle array changes correctly (in particular
changes with the size of the array). Behavior now allows for array
changes with different sizes, and should keep appropriate scroll
positions.

Resolves: aurelia#106, aurelia#98, aurelia#89
Previous behavior did not handle array changes correctly (in particular
changes with the size of the array). Behavior now allows for array
changes with different sizes, and should keep appropriate scroll
positions.
@AStoker
Copy link
Contributor Author

AStoker commented Mar 31, 2017

This is an initial PR to test out functionality. I'd like for a few others to look at it to confirm it works for them in their use cases before merging.
@altenstedt, could you try and pull down this PR and test it out?
@EisenbergEffect, when you have a minute, if you could give the code a look over as well.
I'm out for the weekend, but I wanted to get something out for review as soon as I could. That being said, I'd like to make the tests a bit more robust, and I'll have that in a separate commit on Monday (most likely).

@altenstedt
Copy link

I have tested your PR on our code and it looks good. I have had no issues so far. Looks like this resolves at least #106.

@AStoker
Copy link
Contributor Author

AStoker commented Apr 3, 2017

@altenstedt, so this looks good except for tables right now. The way that we have buffer elements outside the table element has been bothering me for a while, and it starts to interfere here when we are recalculating heights (initial issue that caused buffer elements to be moved: #46).
In regards to the other issues, I'm still doing some testing, but they may require a separate commit. I know at least #89 should be solved, #87 and #98 still require more testing to confirm. I'll be working on it tonight though.

@altenstedt
Copy link

We do not use virtual-repeat.for with tables, so that's why I have not seen issues related to tables.

@AStoker
Copy link
Contributor Author

AStoker commented Apr 3, 2017

Sounds good. I just want to make sure that I don't break anything more than it already is, I can come back later and fix up the table strategy.

Help virtual repeat to handle scrolling with tables a little more gracefully. Does not fully resolve all table scrolling issues, but helps make them less severe.
@AStoker
Copy link
Contributor Author

AStoker commented Apr 3, 2017

@altenstedt, made a few modifications to help tables be a bit more graceful (there's work to be done, as they're messed up as is, but the last commit should help). Just want to make sure it's still working for you with the changes though.

@AStoker
Copy link
Contributor Author

AStoker commented Apr 3, 2017

@EisenbergEffect, I think we're good for a review. These commits fixed a lot of bugs (as is the case when something is still in beta, as this tool is). Tests are all passing, and I tested a bunch of different use cases on my machine, hopefully I covered all the bases (but there's always the edge cases).
We will need to address some of the table rendering issues eventually (probably going to need some help from @martingust to explain a few changes made last year).

@EisenbergEffect
Copy link
Contributor

@AStoker You know more about the virtualization implementation than I do at this point :) If you feel good about it, go ahead and merge. Let me know when it's ready and I'll get a release out. Thanks for the hard work!

@EisenbergEffect
Copy link
Contributor

@martingust If you have a chance to look at this, that would be great. Thanks!

@AStoker AStoker requested a review from martingust April 4, 2017 17:28
@AStoker AStoker self-assigned this Apr 4, 2017
@AStoker
Copy link
Contributor Author

AStoker commented Apr 5, 2017

Well, let's give this a whirl :) We're in Beta for a reason

@AStoker AStoker merged commit ab22140 into aurelia:master Apr 5, 2017
@AStoker
Copy link
Contributor Author

AStoker commented Apr 5, 2017

@EisenbergEffect, ready as we'll ever be for a release :)

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.

3 participants