-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add payment card #42771
Add payment card #42771
Conversation
@dukenv0307 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] |
@shawnborton hey! can i get a review for add payment card screen, please? It maybe some not correct margin between elements - without dev mode hard to check(picked the same as we had for other add debit card screen - 20px). I requested 10 times for dev mode in figma - lol ![]() ![]() |
}; | ||
|
||
function IconSection({icon, iconContainerStyles}: IconSectionProps) { | ||
function IconSection({icon, iconContainerStyles, width = variables.iconSection, height = variables.iconSection}: IconSectionProps) { |
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.
Rather than using 68x68 here, should we use 48x48 like we do for most other simple illustrations in product? cc @dannymcclain @dubielzyk-expensify for thoughts there. This way we don't need to create a new icon size variable either.
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.
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, we typically use 48x48 everywhere in the product these days for this type of illustration, so we might want to be consistent here as well. Let's see what the design team thinks.
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 I'm aligned using 48x48 here
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.
Okay cool, @narefyev91 can we try the icon using our standard 48x48 size please? Thanks!
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 - will update
yes, but the scroll should be flush with the bottom of the view. |
The padding/spacing looks pretty good to me, but looping in @Expensify/design for some eyes too. |
@shawnborton like here Screen.Recording.2024-05-31.at.12.11.31.mov |
ohhh got what you are saying! Screen.Recording.2024-05-31.at.12.15.29.mov |
Exactly! |
yeah! cool fixed! Screen.Recording.2024-05-31.at.12.19.14.mov |
Much better, thanks! |
@rushatgabhane is going to review! |
Yeah - it was fixed here |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeMacOS: Chrome / SafariScreen.Recording.2024-06-04.at.22.18.29.mov |
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.
@narefyev91 im unable to click "I accept" checkbox
Screen.Recording.2024-06-03.at.12.45.48.mov
Yeah I faced the same issue when open dev menu in the bottom. Let me check - probably something from currency select is overlapping |
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
This reverts commit 09724ef2fd252eb2b3a6891b12f7c45b01c184e5.
e038f25
to
0652324
Compare
shouldShowPaymentCardForm={shouldShowPaymentCardForm} | ||
addPaymentCard={addPaymentCard} | ||
showCurrencyField | ||
isDebitCard |
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.
isDebitCard |
The form on this page should be the payment card, not debit card/wallet
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.
@amyevans are you sure?
Based on how was implemented WorkspaceOwnerPaymentCardForm - i see that it used debit card translation
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.
Brought the question to Slack
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.
@amyevans updated
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.
Looks great!
✋ 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 production by https://github.com/luacmartins in version: 1.4.81-11 🚀
|
Created follow up polish issue related to this: #44051 |
@@ -67,7 +67,7 @@ function WorkspaceOwnerChangeWrapperPage({route, policy}: WorkspaceOwnerChangeWr | |||
Navigation.navigate(ROUTES.WORKSPACE_MEMBER_DETAILS.getRoute(policyID, accountID)); | |||
}} | |||
/> | |||
<View style={[styles.containerWithSpaceBetween, styles.ph5, error === CONST.POLICY.OWNERSHIP_ERRORS.NO_BILLING_CARD ? styles.pb0 : styles.pb5]}> | |||
<View style={[styles.containerWithSpaceBetween, error !== CONST.POLICY.OWNERSHIP_ERRORS.NO_BILLING_CARD ? styles.ph5 : styles.ph0, styles.pb0]}> |
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.
Removing styles.pb5 caused the issue #44825 where the Transfer Owner's 'Continue' button was missing a bottom margin.
Details
Implement “Add payment card” screen (UI)
Generally build new component for rendering any kind of Adding payment card form.
Replaced form for Wallet debit card, Owner workspace debit card and Add payment card for subscription
Fixed Issues
$ #38634
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop