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 layout measure, close #101 #102

Merged
merged 1 commit into from
Sep 10, 2017
Merged

Conversation

nihgwu
Copy link
Collaborator

@nihgwu nihgwu commented Sep 8, 2017

I wrapped measureWrapper into InteractionManager to resolve a issue I mentioned in #80 , and I did that change in #80 too, but then I removed it for a simple implementation , now it seems that onLayout is still unreliable, see #101

so I make this change again, ping @chetstone

@nihgwu nihgwu merged commit 31cb333 into master Sep 10, 2017
@nihgwu nihgwu deleted the neo/fix-position-regression branch September 10, 2017 13:27
@chetstone
Copy link
Collaborator

@nihgwu This completely breaks the Sortable example. Please take a look. When dragging any item, the target slot does not move. The top item cannot be moved anywhere, when released it just pops back to the top. Other items, when dragged and released, always move to the top.

@nihgwu
Copy link
Collaborator Author

nihgwu commented Sep 11, 2017

@chetstone Sorry I didn't test my fix with Sortable example because it's RN version is too old, so I tested in my own app and it works well, I've came up with a patch, but I'm wondering that should we upgrade the RN version used by the example?

@computerjazz
Copy link
Contributor

can confirm the same behavior as @chetstone
RN 0.47.1
buggy-sort

@nihgwu
Copy link
Collaborator Author

nihgwu commented Sep 11, 2017

@computerjazz thanks, I've confirm it on Android, the measure returns nothing, wrap it in setTimeout would help, I'll find a better way to resolve it, thanks

@chetstone
Copy link
Collaborator

@nihgwu In general, I think the more tests we do, the better for making robust software. Our users have many different environments, different versions of RN, some using react navigator, others with react-native-router-flux, others with no navigator, some with headers or not, embedded within other components or not, etc. I don't think testing with one app is sufficient.

As far as running the Sortable example, just do an npm or yarn install and the right version of RN will be installed. I think the main problem is that the Sortable example is not automated. It takes time to build, install and operate it manually to test it out. I wonder if there's a way to automate it?

Regarding whether to upgrade Sortable to use a newer version of RN, I can think of arguments on both sides. On the one hand, it's good to keep it as it is for regression testing and to enhance the diversity of testing environments to increase the robustness of the code.

On the other hand, most new adopters of the component will be on newer versions of RN, and it's important to make sure everything works well in that environment.

If we could automate, we could have both. What do you think?

nihgwu added a commit to nihgwu/react-native-sortable-listview that referenced this pull request Sep 14, 2017
nihgwu added a commit that referenced this pull request Sep 14, 2017
eduardojunio added a commit to eduardojunio/react-native-sortable-listview that referenced this pull request Sep 14, 2017
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