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

Configure the repo for public release #2

Merged
merged 12 commits into from
Oct 25, 2016
Merged

Configure the repo for public release #2

merged 12 commits into from
Oct 25, 2016

Conversation

akshaybhalotia
Copy link
Contributor

No description provided.

@akshaybhalotia
Copy link
Contributor Author

@pronav Please review the full code, mostly JS since this is repo is going to be public after this merge.

@captn3m0 Please make a thorough read and suggest changes to the templates in repo, namely README, code_of_conduct, LICENSE and CONTRIBUTING.

Copy link
Contributor

@captn3m0 captn3m0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments. Looks good to me otherwise.

Can we also do our sample application in react native?


[integrations]: https://razorpay.com/integrations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe point to github as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a call. /integrations provides more options (at least more visually organized) than github.

npm uninstall react-native-razorpay
npm install
react-native link
rm -r ./node_modules/react-native-razorpay/ios/Razorpay.framework
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

silly question, but assuming our .framework file is public, can we clone it as part of the setup script here and use that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.framework isn't (and shouldn't be) directly hosted anywhere. Its inside a zip file, which is named by version. We can cURL it and unzip it.

Only one downside: We'll have to change versions every time there's a new release. But shouldn't be a big thing since we'll have to update .framework in this repo also with every release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@captn3m0 I tried doing this today and observed that this script uses a lot of network requests if we add curl to it, while the purpose of this script is to reduce development time and reload plugin code from local setup only while developing plugin. Hence, wouldn't do any benefit IMO.

Instead I added it to npm's postinstall script, so that users need not worry about downloading and replacing the framework, as mentioned in steps 1, 2 and 3 of Linking iOS SDK section of Readme.

@akshaybhalotia
Copy link
Contributor Author

By doing our sample application in React Native, do you mean we have an additional sample app? Or replace the existing razorpay-android-sample-app and razorpay-ios-sample-app with a single sample app made in React Native?

@captn3m0
Copy link
Contributor

Not replace, have another sample app. I see that there is an example directory, does that work for this usecase already?

- Limits lines to 80 characters
- Adds title to all links
- Improvements in backtick formatting
- Moves all links to the bottom
- Also sorts the links alphabetically
@akshaybhalotia
Copy link
Contributor Author

Yes, it serves the same purpose. Should that be explicitly mentioned somewhere if its not clear by the language?

## Usage

Sample code to integrate with Razorpay can be found in
[index.js][index.js] in the included example directory.
Copy link
Contributor

@selvagsz selvagsz Oct 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love to see the code snippet in usage section in addition to the example

@selvagsz
Copy link
Contributor

selvagsz commented Oct 24, 2016

Just checked the code & the api. My concerns are that the consumers shouldn't worry about subscribing/unsubscribing to the success/error event listeners

So instead of

class example extends Component {
  componentWillMount() {
    rzpEvents.addListener('onPaymentError', (data) => {
      alert("Error: " + data.code + " | " + data.description)
    });
    rzpEvents.addListener('onPaymentSuccess', (data) => {
      alert("Success: " + data.payment_id)
    });
  }

  render() {
    return (
      <View style={styles.container}>
       <TouchableHighlight onPress={() => {
        var options = {
          description: 'Credits towards consultation',
          image: 'https://i.imgur.com/3g7nmJC.png',
          currency: 'INR',
          key: 'rzp_test_1DP5mmOlF5G5ag',
          amount: '5000',
          name: 'foo',
          prefill: {email: 'pranav@razorpay.com', contact: '8879524924', name: 'Pranav Gupta'},
          theme: {color: '#F37254'}
        }
        RazorpayCheckout.open(options)
       }}>
      <Text style = {styles.text}>Pay</Text>
    </TouchableHighlight>
    </View>
    );
  }

  componentWillUnmount () {
    rzpEvents.remove();
  }

}

So, I would prefer the public apis to be

RazorpayCheckout.open(options, successCallback, errorCallback)

// or a promise-based api (much preferable)

RazorpayCheckout.open(options).then((response) => {
  // handle success
}).catch((error) => {
  // handle error
})

@selvagsz
Copy link
Contributor

Also, namespace the events that are being emitted in order to avoid any possible conflicts

@selvagsz
Copy link
Contributor

selvagsz commented Oct 24, 2016

@akshaybhalotia Don't lock the peerDependency to a particular version. It should be >=.
Better let's remove the peerDependency from package.json.

Many RN packages removed the peerDependency since the comparator doesn't include rc versions
Kureev/react-native-blur#44

@akshaybhalotia akshaybhalotia mentioned this pull request Oct 25, 2016
@selvagsz
Copy link
Contributor

lgtm 👍

@selvagsz selvagsz merged commit 2eac0a3 into master Oct 25, 2016
@selvagsz selvagsz deleted the f/release_config branch October 25, 2016 13:28
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.

4 participants