benoitvallon / react-native-nw-react-calculator

Mobile, desktop and website Apps with the same code
MIT License
5.21k stars 865 forks source link

Android App #1

Closed dsibiski closed 8 years ago

dsibiski commented 8 years ago

I only need to add 2 small Android specific things, the keyAction button border radius wouldn't show on the TouchableHighlight and the text inside of the "Screen" wasn't lining up properly. That's really good as everything else was left untouched and just worked out of the box! :+1:

In order to share the same code without needing to just straight copy/paste everything in the *.ios.js files, I created *.native.js files and import them via the individual *.ios/android.js files.

This feels a little messy since it adds so many extra files.

Another way we can do it is to let the plain AppRender.js files be the "native" ones (both for iOS & Android) and rename the Web & NW.js files to something like AppRender.web.js. Doing it this way, we'd have less files but we'd probably have to change the Webpack config & gruntfile to account for the change.

Any thoughts?

If you like this and we come to an agreement on the file structure, then I can fill in the blanks on the README as well. :smiley:

android-app

dsibiski commented 8 years ago

Whoops, looks like my renaming of the index.ios.js file broke the Travis build...

benoitvallon commented 8 years ago

Nice PR

The TouchableOpacity is probably a better choice for this type of button. About the alignItems: Platform.OS === 'android' ? 'center' : 'flex-end' although I do not like this kind of thing (I started the project with the idea of avoiding such things otherwise the goal of sharing the code would have been biased) I think that in this particular case it is probably due more to my flexbox implementation which I think can be optimized, so it is totally valid.

I like the file structure that you chose. It adds many files but it allows to keep the existing build system and when you get it, it is very logic and easy to remember.

Yes, please update the readme with all these amazing news things. We now have 4 Apps in one :smiley:

I didn't know how react native for android worked before I had a look at your code. I didn't think it was so similar to the original react native for iOS otherwise I would have probably added it from the beginning :stuck_out_tongue_winking_eye: I think the sdk/emulator installation was the hardest part.

Anyway, thanks for doing it

dsibiski commented 8 years ago

@benoitvallon Ok excellent. I should be able to get that updated sometime today. :+1:

cjb commented 8 years ago

We've been using a patch to react-native's packager to get foo.mobile.js (just like your foo.native.js) working natively, so that there can be separate files for each of:

foo.ios.js # IOS specific foo.android.js # Android specific foo.mobile.js # IOS or Android foo.web.js # Web only foo.js # Shared between Web, IOS, and Android

Here's the patch! But the PR didn't get merged. Might be worth chiming in on it to say that you'd like to use something like this too. :)

https://github.com/facebook/react-native/pull/3641

dsibiski commented 8 years ago

@cjb Very cool, thanks for the reference! :+1: Actually, I've noticed there that the RN packager will properly ignore files with *.web.js (I didn't know that)...I wonder if Webpack will be able to work with that out of the box, I think I'll experiment with it today.

benoitvallon commented 8 years ago

@cjb thanks again for your idea by the way ;) it worked great.

The native.js extension would be great for this project and would avoid some boilerplate code but I have no idea how it would work for a bigger project. I am worried that the multiple fallbacks would create confusion for new people using RN and their point about any future implementation of RN for other platform would make RN even harder to get for new people. That's just my quick thoughts on it I haven't thought of it so much.

I am wondering if it might be possible to create a npm package that handles the boilerplate code at least for this project, because your fallback requirements are more complex.

benoitvallon commented 8 years ago

@benoitvallon Ok excellent. I should be able to get that updated sometime today. :+1:

awesome, thanks

dsibiski commented 8 years ago

@benoitvallon Ok, updated. I tried to cover everything and explain a little about the .native.js files. Please let me know if you want me to change anything. :smiley:

benoitvallon commented 8 years ago

Awesome work :+1:

dsibiski commented 8 years ago

Thanks! Although it's only a small contribution compared to the awesomeness that you created first. :)