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 issues in beta release #47

Merged
merged 4 commits into from Mar 26, 2020
Merged

Fix issues in beta release #47

merged 4 commits into from Mar 26, 2020

Conversation

@igstewart3
Copy link
Contributor

igstewart3 commented Mar 25, 2020

This PR fixes some issues in the 4.1.0-beta release:

  • Creating Android classes from JS objects has been fixed.
  • index.js now checks which module is available. It then adds the extra functions and exports it for the module.
  • Uses of 'Carnival' in the examples have been removed.
@igstewart3 igstewart3 requested review from raspygold and j-stokes Mar 25, 2020
purchaseItems.push(new Carnival.PurchaseItem(item));
});
return new this(purchaseItems);
module.exports = SailthruMobile;
}

This comment has been minimized.

Copy link
@j-stokes

j-stokes Mar 25, 2020

Only thing I would suggest is to have module.exports = ... in one place.

So, rather than having it in each block

if (Carnival) {
  ...
  module.exports = Carnival;
}

...

if (SailthruMobile) {
  ...
  module.exports = SailthruMobile;
}

you could have it at the bottom of the file and use a ternary instead. eg.

  module.exports = Carnival ? Carnival : SailthruMobile;

This comment has been minimized.

Copy link
@igstewart3

igstewart3 Mar 25, 2020

Author Contributor

👍

Ian Stewart
Copy link

raspygold left a comment

Changes look good to me

@igstewart3 igstewart3 merged commit 1d1d81f into master Mar 26, 2020
2 checks passed
2 checks passed
ci/bitrise/3fe3ad4df881bac6/pr Passed - carnival-sdk-react-native
Details
ci/bitrise/3fe3ad4df881bac6/push Passed - carnival-sdk-react-native
Details
@igstewart3 igstewart3 deleted the beta-fixes branch Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.