Fix animation

[chetstone]

This fixes Issues #48 and #51, generally improving animation quality. There were a number of small bugs that mainly caused tracking to be off between the moving row and the target.

Also added a complete example app, Sortable.

This PR also includes code originally proposed in PR #49. This can be tested by using list3 or list4 in the test app.

[superandrew213]

@chetstone I just tried it but it's still not working for me.

[chetstone]

Sorry about that. Can you tell me more about what's happening? The gif you attached doesn't play well on my system. It looks like you're clicking on item 2 and it's disappearing. Is this your own app or the sample app I included? Perhaps you could try the sample app and see what's different..

[superandrew213]

Yes that's it. When I press a row, it disappears and pops up on top later. This is the sample app you included.

[chetstone]

Check to see if you're using the right version of the component. I use a file:../ version specification in package.json and yarn has a nasty habit of installing an old one from its cache.

cd .../react-native-sortable-listview/Sortable
diff ../index.js node_modules/react-native-sortable-listview/index.js

If they're different, force yarn to install the right version like this:

rm -fr node_modules
yarn cache clean
yarn install

[superandrew213]

Just checked the files and it's installing the right one.

[chetstone]

@superandrew213 I just tried to reproduce your issue by cloning the repository into a new directory and starting fresh. I used the following sequence of commands:

git clone https://github.com/chetstone/react-native-sortable-listview.git
cd react-native-sortable-listview
git checkout fix_animation
cd Sortable
yarn cache clean
yarn install
watchman watch-del-all
npm start --reset-cache
#react-native run-ios
[open ios/*proj in xcode.]
[set Signing Team in xcode for Sortable and SortableTests]
[Product/Run]

Everything worked fine. Do you have any idea what could be different between your environment and mine? I am on MacOS 10.11.6, npm version 3.10.9, node version v7.2.0, xcode version 8.2.1, and yarn version 0.21.2. I'm running on the iPhone 7 simulator running iOS 10.2. I also tried it on an actual device, iPhone 6+/ iOS 10.2. And on an Android 6.0 Samsung Galaxy S5.

Here is the GIF of my results:

[superandrew213]

@chetstone I can confirm that it's working well when I do a fresh project. However if I just drop the SortableListView Component in my project (without even installing the package) and also add the Sortable example it doesn't work. The active row just disappears. Might be my project specific through. Other than that PR looks good.

[superandrew213]

If I set the Sortable example component as my root component in my project it works. However if I use the Sortable component in a stacked router scene then it doesn't. Must have something to do with the fact that it is stacked?

[chetstone]

What do mean by "stacked"? It works fine in my app which uses react-native-router-flux. The basic configuration is:

          <Router >
              <Scene key="root" hideNavBar={true}>
                  <Scene key="this" component={This} title="This" />
                  <Scene key="that" component={That} title="That" />
                  <Scene key="counterlist" component={CounterList} title="CounterList" />

Where CounterList is basically:

render: function () {
    return (
        <Header />
        <SortableListView
             data={ my app data }
             renderRow={ my render row }

... etc

(Note that I've tested this on Android only. Although the Sortable example app works fine on iOS I haven't tried it with my own app because my iOS app doesn't use this component)

[superandrew213]

@chetstone change one of your scene directions to vertical. This can't be the initial scene. The issue seems to be with the pageY value when you call measure.

On a stacked scene with vertical direction the pageY value seems to be the height of all the scenes before it.

[chetstone]

Andrew,

I’m still not understanding what you mean. Could you provide some code to demonstrate? Perhaps a simple modification to the Sortable example that demonstrates the problem?

thanks

On Feb 28, 2017, 16:28 -0700, Andrew Shini notifications@github.com, wrote:

@chetstone change one of your scene directions to vertical. This can't be the initial scene. The issue seems to be with the pageY value when you call measure. On a stacked scene with vertical direction the pageY value seems to be the height of all the scenes before it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[superandrew213]

Try this example.

Issue 1: Active row disappears when on a nested scene with direction vertical

If you change the Sortable scene to direction="horizontal" it doesn't happen.

Issue is here

Log the pageY value for direction="vertical" and direction="horizontal" and you will see the difference.

Issue 2: Small lists that don't fit the whole screen don't behave as they should Check out SmallList scene

[chetstone]

@superandrew213 Thanks very much for the example code and the heads up on the problem with the short list. I've just pushed a fix for that. Regarding the problem with the router vertical animations, that's beyond the scope of what I was trying to do with this PR, and it's not a regression --- the same problem occurs with the released version. So please file another issue for that problem and we can take a look after this PR is merged.

[superandrew213]

Awesome thanks for fixing the short list issue. Yes the router issue is not part of this PR. I just pointed it out to let you know that it wasn't your PR that was causing the issue I was having.

[chetstone]

@milesibastos @deanmcpherson Any chance of getting this merged? It looks like it will also fix #50 and #58 and possibly #44 and #39.

It's still not perfect--- I've seen the following minor issues when integrating this in my app (but I cannot reproduce them using the example app, which seems to work perfectly)

1. The row above can still drop down when selecting an item to move, but it happens infrequently (maybe 10% of the time) and as soon as you start moving the item, everything moves into the correct position.

2. Sometimes when you slide an item back into its original position a partial duplicate of itself will flash behind in the target slot it is being moved over.

Anyway IMHO it works much better overall. And I may be seeing these things in my app because I'm using an older version of RN: 0.33.1.

[chetstone]

Maintainers, please hold off on merging this PR. I have tracked down the problem I have in my app, and have managed to change the example to demonstrate the problem. I will be pushing a fix shortly.

When the height of the row is not an integer, cumulative floating point errors in the checkTargetElement loop can cause the comparison to go awry for certain indexes, and when that item is pressed, the above item will slide down.

[chetstone]

All set. Please merge. @milesibastos remember you said PR is welcome!! (#51)

[joshuapinter]

This looks fantastic. Great job both @chetstone and @superandrew213 for tracking this down and making the fixes.

@chetstone, do you have a branch where all of these fixes are contained in that I can use until @milesibastos merges this in to Master and releases a new npm version?

Thanks!

[joshuapinter]

@chetstone Nevermind, didn't realize we were on a PR already. :) Looks like everything is nicely contained in the fix_animation branch. 👍

[joshuapinter]

@chetstone Your branch is much better than master. I found a weird issue where if it's contained below a view (in my case a simple image), it doesn't work well. Is there anything you can think of that is dependent on being at the top of the screen? Absolute positioning or something? Thanks.

[chetstone]

@joshuapinter Thanks for the feedback. I don't know if I understand the problem. Your gif shows the image below the list and above. Is the problem that the image moves to the other side of the screen or are you just trying to show that the list works OK when the image is below?

How is your code different from the Sortable example? The example does have a view above the list, in this case just some text with a height of 64. But I tried changing the height and also replacing the text with an image and I can't reproduce the problem. The example uses flex positioning, perhaps you're using absolute positioning in your code?

Note- I don't know if it's related but I'm looking into another minor issue --- if the list has been scrolled even slightly and you grab a list item near the top, the list will start scrolling up and it will look like the upper item is dropping down --- actually, the whole list is starting to scroll. (If I find a solution I will file another PR-- I don't want to delay merging of this PR by adding anything more to it.)

[joshuapinter]

@chetstone Sorry for the confusion. Yes, I was just showing that when the image is above and the sortable ListView is below, it doesn't work. A long press just makes the element disappear. But when the ListView is above, it works perfectly fine.

I'll keep digging here and see if I can drum up any clues.

Good call with not delaying the PR merge. If I find anything specific I'll create a new Issue for it.

Thanks again for all your hard work with this.

[chetstone]

@joshuapinter I think I was able to reproduce your problem, or something similar. It seems there is a race condition when measuring the view that can cause incorrect results on some devices/configurations. In componentDidMount, there is a call to measure() from a setTimeout of 1ms. But the docs imply that rendering may not be finished yet, thus the measurements can be incorrect.

Although I could not replicate the issue yesterday, today, using an older android device in DEV mode, this.wrapperLayout.pageY was always 64, no matter what component was rendered above the list. Changing the timeout to 100ms "fixed" the issue. I'm not sure of the correct way to fix this, but could you try it out to see if it fixes your issue? I have pushed this hack onto this branch.

(BTW looks like you're doing good work on react-native-sortable-list!

[joshuapinter]

Thanks for looking into this @chetstone. And yes, I spent some time on getting Android up to speed on react-native-sortable-list! - working and looking great now!

We're under a deadline to get this project wrapped up but when things settle down I'll try and come back to check this out. 👍

[deanmcpherson]

Hey @chetstone, I don't have the time to maintain this any more, would you like to be a contributor?

[chetstone]

OK, sure. Set me up.

On Mar 7, 2017, 18:23 -0700, Dean McPherson notifications@github.com, wrote:

Hey @chetstone, I don't have the time to maintain this any more, would you like to be a contributor? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[deanmcpherson]

Done, let me know if you have an npm account and I'll add you to the npm repo as well.

[chetstone]

Thanks Dean,

My shiny new npm account is “chetstone”

Cheers

On Mar 7, 2017, 18:58 -0700, Dean McPherson notifications@github.com, wrote:

Done, let me know if you have an npm account and I'll add you to the npm repo as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[deanmcpherson]

Thanks @chetstone, you're all set up.