react-native-chrome-custom-tabs icon indicating copy to clipboard operation
react-native-chrome-custom-tabs copied to clipboard

Update for newer versions of React Native (and some other goodies)

Open artdent opened this issue 8 years ago • 13 comments

Hi! I'm using this module in my project, and I wanted to share some improvements. I could make separate PRs for each one if you want, or you can just take a look at all of them here.

The two most useful are:

  • Support newer versions of React Native
  • Add a hook for consumers to customize the CustomTabsIntent (from native code, although exposing options in JS would be nice too)

If you happen to not be interested anymore, I'm happy to maintain my own fork, but it'd be nice to get these changes into a released version on NPM.

artdent avatar Feb 21 '17 16:02 artdent

Side effect of these improvements is that react-native link will install the correct gradle configuration and add the ChromeCustomTabsPackage to the package list automatically 😄

joshfriend avatar Mar 01 '17 20:03 joshfriend

@artdent Thank you so, so much for this pull request! I apologize on the silence; I wasn't aware that GitHub doesn't automatically notify you of activity on your own repos.

I'm going to take a look at this PR this evening, but from what I see I don't have any major objections. Does this have any breaking API changes? I'm okay putting it out as a major semver update, but only if need be.

Again, thank you so much!

dstaley avatar Apr 25 '17 14:04 dstaley

@artdent Could you also give me a bit of background behind the changes in https://github.com/dstaley/react-native-chrome-custom-tabs/pull/2/commits/cfd640446c0319136d4ae4b1c33bbb52f9343f85?

dstaley avatar Apr 25 '17 23:04 dstaley

Yay, thanks for taking a look!

Good call about backward compatibility. I just pushed a change to make sure the old constructor for ChromeCustomTabsPackage still works. With that, I think this PR is now backward-compatible. (The JS changes didn't affect the exposed API.)

I also just pushed a fix to 4753b75, which should have deleted the platform-independent JS entry point.

The point of cfd6404 is to expose the ability to customize the look and feel of the Chrome custom tab. Here's what its usage looks like in my project, for example:

        new ChromeCustomTabsPackage(new CustomTabsIntentEditor() {
          @Override
          public void customize(Context context, CustomTabsIntent.Builder builder) {
            Bitmap backIcon = BitmapFactory.decodeResource(context.getResources(),
                R.drawable.ic_arrow_back);
            builder
                .addDefaultShareMenuItem()
                .setShowTitle(true)
                .enableUrlBarHiding()
                .setToolbarColor(context.getResources().getColor(R.color.primary_dark))
                .setStartAnimations(context, R.anim.slide_in_left, R.anim.slide_out_left)
                .setExitAnimations(context, R.anim.slide_in_right, R.anim.slide_out_right)
                .setCloseButtonIcon(backIcon);
          }
        })

It would be friendlier to expose these knobs to JavaScript, but some of them require referencing native code anyway, so I went with the simpler solution.

artdent avatar Apr 26 '17 20:04 artdent

Hello everyone, thank you all for making this (and updating it), I'm on RN 0.43, how long till we can get this on npm? :)

kristoff-it avatar May 01 '17 11:05 kristoff-it

@artdent Okay, finally had the chance to sit down and give this a proper review! I remembered playing around with customizing the custom tab, so I wanted to see how I did that. It looks like I was passing a config object into the launchCustomTab function. Since we can expose this functionality completely via JavaScript, I'd rather not merge in the native method of customizing it. Here's a gist of how I was doing it previously. It only allowed the toolbar color to be customized, but I'm sure we can figure out a way to support all the simple customization options. I think animations might be a bit trickier, so I'd love to hear your thoughts on that.

Other than that, I think this is good to merge! Since I never released a 1.0.0, I'd like to tag this as such :) Thanks again for submitting this! I really, really appreciate it!

dstaley avatar May 01 '17 23:05 dstaley

🎉

kristoff-it avatar May 01 '17 23:05 kristoff-it

Cool. That all sounds good.

It looks like specifying animations and bitmaps from JavaScript should be possible with some difficulty. It can be done in a manner similar to this code from the react Image implementation:

      int id = context.getResources().getIdentifier(name, "drawable", context.getPackageName());

source

That said, since you need to be using Android Studio to manage those resources anyway, I personally would find it more convenient to customize the intent on the native side. I would rather make available both JavaScript props and a native hook that can reference CustomTabsIntent.Builder directly. But if you think that's overcomplicated then I can drop cfd6404 from this PR.

artdent avatar May 03 '17 14:05 artdent

If we can support a limited set of customizations from JavaScript, and then the full power of the SDK from Java, that'd be :fire:!

Forgive me as I'm not a Java/Android developer, but would customizing the IntentEditor happen in MainActivity.java? If so, would you need to add an additional import in addition to importing ChromeCustomTabsPackage to bring in CustomTabsIntentEditor? If so, could you add that to the README?

For customizing from JS, could we precreate the mResourceDrawableIdMap, and only apply animation if the supplied string key is present in the map? Something like this on the JS side:

client.launchCustomTab('http://i.imgur.com/xjdem.gif' {
  tabEntranceAnimation: 'slide_up',
  activityExitAnimation: 'fade_out',
  tabExitAnimation: 'slide_down',
  activityEntranceAnimation: 'fade_id'
});

and then this on the Java side (please forgive me if this doesn't compile, I don't have an IDE handy):

Map<String, Integer> mResourceDrawableIdMap = new HashMap<String, Integer>();
mResourceDrawableIdMap.put("slide_up", R.anim.slide_up);
mResourceDrawableIdMap.put("slide_down", R.anim.slide_down);
mResourceDrawableIdMap.put("fade_in", R.anim.fade_in);
mResourceDrawableIdMap.put("fade_out", R.anim.fade_out);

if (config.hasKey("tabEntranceAnimation") && config.hasKey("activityExitAnimation")) {
  String tabEntranceAnimationKey = config.getString("tabEntranceAnimation");
  String activityExitAnimationKey = config.getString("activityExitAnimation");
  if (mResourceDrawableIdMap.hasKey(tabEntranceAnimationKey)
      && mResourceDrawableIdMap.hasKey(activityExitAnimationKey)) {
    builder.setStartAnimations(
      mContext.getApplicationContext(),
      mResourceDrawableIdMap.get(tabEntranceAnimationKey),
      mResourceDrawableIdMap.get(activityExitAnimationKey)
    );
  }
}

The downside to this approach is that it would grow unwieldy as the number of animations grow, but I think that we can provide just a small set (maybe 8?), and then allow anything outside of those to be done in Java natively.

dstaley avatar May 03 '17 15:05 dstaley

So is this available in npm now?

vivnau123 avatar May 14 '17 14:05 vivnau123

@vivnau123 No, not yet. If you need this functionality in the meantime, use npm to install the package from the pull request.

dstaley avatar May 14 '17 17:05 dstaley

I was using the package with the pull request in rn0.35 and it was working fine. But I upgraded to 0.43.4 and it gives module not found error with the pull request even though the module is there in my node modules

vivnau123 avatar May 14 '17 19:05 vivnau123

Will this PR also work on RN > 0.40?

itinance avatar Aug 10 '17 10:08 itinance