-
Notifications
You must be signed in to change notification settings - Fork 13
Prototype development/inline android component #383
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
Prototype development/inline android component #383
Conversation
...oid/src/main/java/com/shopify/reactnative/checkoutsheetkit/CustomCheckoutEventProcessor.java
Outdated
Show resolved
Hide resolved
...oid/src/main/java/com/shopify/reactnative/checkoutsheetkit/CustomCheckoutEventProcessor.java
Outdated
Show resolved
Hide resolved
...t-kit/android/src/main/java/com/shopify/reactnative/checkoutsheetkit/RCTCheckoutWebView.java
Outdated
Show resolved
Hide resolved
|
Remaining task to be completed here is to update the gradle config to ensure that we are able to build against local android package without modifying lib (JVM_1_8 -> JVM_17) |
2e352cc to
37f95c9
Compare
|
|
||
| @Override | ||
| public void onComplete(@NonNull CheckoutCompleteEvent event) { | ||
| public void onCheckoutCompleted(@NonNull CheckoutCompleteEvent event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe cart before the horse but these should be onComplete present tense for platform consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah not to worry I'm resolving these locally, had to rebase (this is an old branch)
...oid/src/main/java/com/shopify/reactnative/checkoutsheetkit/CustomCheckoutEventProcessor.java
Outdated
Show resolved
Hide resolved
...t-kit/android/src/main/java/com/shopify/reactnative/checkoutsheetkit/RCTCheckoutWebView.java
Outdated
Show resolved
Hide resolved
westeezy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a few nits but given the holiday and the fact this overall links really solid im approving :)
83e818f to
565c443
Compare
565c443 to
3b9ad9d
Compare
| eventDispatcher.dispatchEvent(new CheckoutEvent(surfaceId, viewId, eventName, params)); | ||
| } | ||
|
|
||
| private class InlineCheckoutEventProcessor extends DefaultCheckoutEventProcessor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kiftio moved back to RCTCheckoutWebView having its own processor so i could remove all the changes to CustomCheckoutEventProcessor (but I renamed it to SheetCheckoutEventProcessor to distinguish them)
...roid/src/main/java/com/shopify/reactnative/checkoutsheetkit/SheetCheckoutEventProcessor.java
Outdated
Show resolved
Hide resolved
| /// resulting in an empty view being rendered. Cannot move setup to constructor as | ||
| /// checkoutUrl is undefined until setCheckoutUrl is called by RCTCheckoutWebViewManager | ||
| /// requestLayout / invalidate were unsuccessful in remedying this | ||
| checkoutWebView.layout(0, 0, getWidth(), getHeight()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to override onLayout ot deal with size changes?
@Override
protected void onLayout(boolean changed, int left, int top, int right, int bottom) {
super.onLayout(changed, left, top, right, bottom);
if (checkoutWebView != null) {
checkoutWebView.layout(0, 0, right - left, bottom - top);
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my initial approach which had the race condition
I've got a comment above that hopefully explains it but the onLayout was executing before setupCheckoutWebView (because the FrameLayout is constructed before react props are passed in) meaning we dont have a view to construct yet
Before:
onLayout (dimensions zero) -> setupCheckoutWebView -> view is zero sized
After:
onLayout (dimensions zero) -> setupCheckoutWebView -> re-measure layout after adding view -> view displays
I tried using requestLayout/invalidate to get android to re-run onLayout but didn't see a subsequent execution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, yeah I was thinking in case the layout changes in some way (orientation?) and we don't resize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - probably worth keeping it still just incase
| private CheckoutConfiguration lastConfiguration = null; | ||
| private final Handler mainHandler = new Handler(Looper.getMainLooper()); | ||
|
|
||
| private record CheckoutConfiguration(String url, String authToken) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just thinking about this, I think support for records might have come after what we've defined for
sourceCompatibility
targetCompatibility
Unless the android gradle plugin supports desugaring
| errorMap.put("recoverable", checkoutError.isRecoverable()); | ||
| errorMap.put("code", checkoutError.getErrorCode()); | ||
| private Map<String, Object> populateErrorDetails(CheckoutException error) { | ||
| Map<String, Object> errorMap = new HashMap<>(Map.of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may want to check when Map.of support was introduced too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...t-kit/android/src/main/java/com/shopify/reactnative/checkoutsheetkit/RCTCheckoutWebView.java
Show resolved
Hide resolved
...t-kit/android/src/main/java/com/shopify/reactnative/checkoutsheetkit/RCTCheckoutWebView.java
Outdated
Show resolved
Hide resolved
...t-kit/android/src/main/java/com/shopify/reactnative/checkoutsheetkit/RCTCheckoutWebView.java
Outdated
Show resolved
Hide resolved
...t-kit/android/src/main/java/com/shopify/reactnative/checkoutsheetkit/RCTCheckoutWebView.java
Show resolved
Hide resolved
9d5e829 to
3a7c499
Compare
kiftio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀

What changes are you making?
This PR integrates the CheckoutWebview thats being exposed in this PR Shopify/checkout-sheet-kit-android#397 so that it can be rendered in an "inline" mode - you'll need to have this branch checked out also to run it
How to test
Run the React Native Android sample app and go to the buy now screen, you should be able to do the same address change flow that occurs in the ios react native app
PR Checklist
Important
Releasing a new version of the kit?
package.jsonfile.Tip
See the Contributing documentation for instructions on how to publish a new version of the library.