-
Notifications
You must be signed in to change notification settings - Fork 100
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
Slightly hacky support for android - Updated #18
base: master
Are you sure you want to change the base?
Conversation
Hi @adamrainsby, can you please explain what the thinking is behind these changes? I see you added the empty onLayout but not sure what the purpose is, is this still a work in progress or does it work with these changes? |
@gr4yscale See this issue for the onLayout thing. facebook/react-native#3282 |
If you want to contribute and get it working with the animation again that would be great. At the moment it literally just shows and hides the content without any animation so it definitely can be improved. |
Hey guys, thanks for moving this forward. I would opt for a solution that retains the animations. @gr4yscale Have you had any luck fixing this? |
@naoufal Just to be clear, the animations still work on iOS but it just opens and closes on android. That is an improvement on it not working at all on android. I think it should be merged in and then we can open an issue for animations on android? |
@adamrainsby you're right. Before merging, could you post a video of the example running on Android? |
82aea03
to
9f89d89
Compare
@naoufal I had to do a bit of work to get the example app running on android. I've updated my PR with those changes. iOS specific components like NavigatorIOS were stripped out for the android version but they are still there when running on iOS. I also updated the version of react native to the latest version because there was a bug where the background would disappear after pressing the accordion. Not sure the exact version this was fixed but for now we can say there is android support from 0.23? |
@naoufal Is this ok? |
@naoufal if this is okay , could you please merge the code and do a release ? i can t use your great module at the moment since my app has to be android / ios compatible. Thanks you ! |
+1 |
@adamrainsby Sorry for being MIA all. @adamrainsby If you rebase this branch, I'll gladly merge it. |
@naoufal : Here is my test of @adamrainsby . Since i have copied directly @adamrainsby fork into my node_modules dir , i had to install react-tween-state and everything works fine ( except animations of course, but may that could be related to a tween effect or duration ? ) .. here is a screen cast : |
@huitiemesens You can put "adamrainsby/react-native-accordion" instead of a version number in the package.json and npm install as usual instead of copy and pasting. |
@adamrainsby thanks for this tips, i m more a composer guy , i didn t know that could be done within a package file |
hi, FATAL EXCEPTION: mqt_native_modules |
In order to keep it clean @naoufal could you please merge this one ? Thanks |
Apologies @huitiemesens, I still need to rebase. I'll try to do it soon. |
@adamrainsby don t worry i already switch for your fork in my package.json , since @naoufal seems afk for a while now ... In order to fix a great bug ( avoiding to compile a signed android apk ) could you please take a look at https://github.com/naoufal/react-native-accordion/pull/33/commits ( its basically renaming index.ios.js to index.js ) ! Thanks you ! |
@huitiemesens I already did that in my fork. Are you still having issues with it? |
@naoufal Rebased, could you merge? |
My last pull request had a bug. This one should work.
#16