-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Bump react-native-wallet to 0.1.10 #70385
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
Bump react-native-wallet to 0.1.10 #70385
Conversation
|
|
|
@stitesExpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
ios/Podfile.lock
Outdated
| - RNFlashList (1.8.2): | ||
| - DoubleConversion | ||
| - glog | ||
| - hermes-engine | ||
| - RCT-Folly (= 2024.11.18.00) | ||
| - RCTRequired | ||
| - RCTTypeSafety | ||
| - React-Core | ||
| - React-debug | ||
| - React-Fabric | ||
| - React-featureflags | ||
| - React-graphics | ||
| - React-hermes | ||
| - React-ImageManager | ||
| - React-jsi | ||
| - React-NativeModulesApple | ||
| - React-RCTFabric | ||
| - React-renderercss | ||
| - React-rendererdebug | ||
| - React-utils | ||
| - ReactCodegen | ||
| - ReactCommon/turbomodule/bridging | ||
| - ReactCommon/turbomodule/core | ||
| - Yoga |
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, all these bumps don't seem right, they don't belong to the PR. I wonder why they weren't included in some previous PRs related to the bumps 🤔
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.
I was also a bit concerned about them. Should I remove them and leave only wallet-related ones?
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.
Personally I'd prefer the removal, and maybe starting a discussion how we should approach the problem. It's not a big deal, but it should be addressed in some way. A simple action that runs pod install and compares the diff should suffice. I think it's been already raised by @sumo-slonik, but in general the approach that we have just seems messy
Co-authored-by: Mariusz Stanisz <mariusz.stanisz@swmansion.com>
5641f3c to
5d0495c
Compare
|
@stitesExpensify could you assign C+ to review this PR? :) |
|
Kicked off the build, @Skalakid I remember this was discussed somewhere but do you have an issue for it? |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
@shawnborton I think most of the things described on your screen attach to websites and email assets, not mobile apps. For mobile apps, the write:
This version of the button is slightly different, the border is thinner, and the background isn't totally black. Also wallet icon and text have a constant size. More info can be found in documentation. To match the design you show. I can try scaling the whole button in React Native, so the text and the icon are bigger, and there isn't that much passing inside |
|
@shawnborton I guess Apple changed PKAddPassButton designs, and now they have different padding and border radius. I've been trying to modify the button, but without much success. The buttons have a restricted design, and I can change only two things: whether it should have an outline and dark frame dimensions. Based on that, iOS creates a button automatically. I tested both PKAddPassButton and AddPassToWalletButton components from Apple, and they return the same different design: The same button can be found in the Stripe library, and it has the same designs as the ones above: So I think this is a correct version for native apps. Not the one from the guidelines. They describe the website and email versions there.
Taking into account all of that, it's impossible to style it "exactly like that" without hacking and changing the official Apple native component. I can change its scale 1.3x to match previous sizes and apply a custom border to match the previous design, and it will look as follows (upper button is mine changed and below we have the previous SVG one for comparison):
Let me know if that works for you, but I personally would follow the consistent design from PassKit that is added in this PR and is created by an official Apple component. cc @mountiny |
|
Yeah, that works for me. I don't want to hold this up anymore. It does feel odd that what we have in product is not looking like the many examples I am seeing from Apple but I'm not sure what else to say at this point. |
|
Seems like that Apple just did not update all the screenshots with the new design. @shawnborton @Skalakid so just to confirm, are there any changes to do here or is the design in the app/ size too that was shared above good to go? |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@shawnborton @mountiny Hello, I've updated button styles on both Android and iOS. That's how it looks now:
Let me know if that works for you :D |
|
cc @Expensify/design for extra eyes but I think this is probably good to ship 👍 |
|
I think so too 👍 |
|
LGTM2 👍 |
stitesExpensify
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.
All you @mountiny
mountiny
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.
Thank you!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.27-0 🚀
|
|
@Skalakid I have tested this in staging and it works fine in iOS, except after adding the card to the Wallet, the button did not change to the |
|
hmmm @mountiny there wasn't any change that could affect that 🤔 I will check it |
|
@mountiny I could reproduce it on prod v9.2.18.-7, and I think it's a problem with the backend response. In the video below, you can see that I'm trying to add the card with wrong.card.is.being.added.MP4 |
|
Hmm I do think in my case it added the correct card |
|
The BE uses the cardID that is passed so if a wrong card was provisioned the App must have sent incorrect cardID then https://github.com/Expensify/Auth/blob/a5f87380694b700ba4d3ed7a8d469ddcc4c7b3af/auth/command/CreateDigitalWallet.cpp#L79 |
|
FYI, posted in the issue here to discuss #71078 (comment) |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.2.27-6 🚀
|











Explanation of Change
This PR bumps the
react-native-walletlibrary to the latest version (0.1.10). It includes support for RN 0.81 and new native "Add to Wallet" button components. In the new version localization prop has been removed, because as specified in the documentation, the language and style are automatically taken care of by the system (on both iOS and AndroidFixed Issues
$ #71078
PROPOSAL:
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/13690
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
N/A
iOS: Native
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
N/A
MacOS: Desktop
N/A