search

Famous / famous-angular

Bring structure to your Famo.us apps with the power of AngularJS. Famo.us/Angular integrates seamlessly with existing Angular and Famo.us apps.
https://famo.us/angular
Mozilla Public License 2.0
1.92k stars 275 forks source link

fix: updating scrollview reseting scroll position #223

Open JonnyBGod opened 9 years ago

JonnyBGod commented 9 years ago

When adding, removing and updating items inside a scrollview it was rebuilding the node from scratch causing it to break the scroll experience by reseting the position to 0.

This way the ViewSequence is reused as according to best practices and the user experience becomes as expected.

I also took the liberty to make it more performant when removing items.

zackbrown commented 9 years ago

Hey @JonnyBGod — thanks for this contribution! This will be a welcome performance enhancement for fa-scrollview. Could you please sign Famo.us's CLA so that we can merge this in? http://famo.us/cla

JonnyBGod commented 9 years ago

Thank you. Will sight the Famo.us http://famo.us/’s CLA.

Give me a couple of days to double check everything as I am still not fully satisfied with how the directive handles surfaces height changes.

On 08 Oct 2014, at 23:03, Zack Brown notifications@github.com wrote:

Hey @JonnyBGod https://github.com/JonnyBGod — thanks for this contribution! This will be a welcome performance enhancement for fa-scrollview. Could you please sign Famo.us's CLA so that we can merge this in?http://famo.us/cla http://famo.us/cla — Reply to this email directly or view it on GitHub https://github.com/Famous/famous-angular/pull/223#issuecomment-58435352.

JonnyBGod commented 9 years ago

CLA signed

zackbrown commented 9 years ago

Alright, thanks! As for managing surface height changes, that should be (and is) the responsibility of the Famo.us layer rather than the Famo.us/Angular layer.

We're about ready to flip the switch on Famo.us 0.3.0 in Famo.us/Angular—0.3.0 has a significantly improved Scrollview, including support for true-sized children. https://github.com/Famous/famous/blob/master/CHANGELOG.md

With that in mind, do you feel good about merging this in?

JonnyBGod commented 9 years ago

Yes I just realised that the height problem is in famous layer. As for version 0.3.0, I tested this PR with 0.3.0-rc and works fine.

With that said, feel free to merge.

On 09 Oct 2014, at 21:42, Zack Brown notifications@github.com wrote:

Alright, thanks! As for managing surface height changes, that should be (and is) the responsibility of the Famo.us layer rather than the Famo.us/Angular layer.

We're about ready to flip the switch on Famo.us 0.3.0 in Famo.us/Angular—0.3.0 has a significantly improved Scrollview, including support for true-sized children.https://github.com/Famous/famous/blob/master/CHANGELOG.md https://github.com/Famous/famous/blob/master/CHANGELOG.md With that in mind, do you feel good about merging this in?

— Reply to this email directly or view it on GitHub https://github.com/Famous/famous-angular/pull/223#issuecomment-58574239.

JonnyBGod commented 9 years ago

Just to leave for future reference.

At the moment with version 0.3.0-rc you it is possible to use surfaces with [undefined, true] size. However, for some reason you have to set it on the surface and cannot use fa-size on the modifier.

Also, it is not currently updating the positioning when the height of a surface changes. I guess we will have to wait for https://github.com/Famous/famous/issues/122 to be replicated on scrollviews.