-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[No QA] 1-6 Card Flow UI #44741
[No QA] 1-6 Card Flow UI #44741
Conversation
@DylanDylann 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] |
src/libs/CurrencyUtils.ts
Outdated
if (amountAsInt === null || amountAsInt === undefined) { | ||
return ''; | ||
} | ||
return convertToFrontendAmountAsInteger(amountAsInt).toFixed(2); | ||
return convertToFrontendAmountAsInteger(amountAsInt).toFixed(withDecimals ? 2 : 0); |
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.
We are implementing a PR to make this change. In that PR, the decimal relies on the currency and I think it will make more sense
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 we will be able to get an amount without decimals then?
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 am not sure what you mean.
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.
In this PR, we will display the decimal relies on the current currency. Then do we need to apply your change here any 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.
yes, because we don't want to display any decimals here - it's for the limit step. I guess that after the changes from this PR you mentioned we will have 2 decimals for USD and for other currencies using 2 decimals.
@mountiny @DylanDylann pushed the last changes |
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.
LGTM
src/pages/workspace/expensifyCard/WorkspaceExpensifyCardPageEmptyState.tsx
Outdated
Show resolved
Hide resolved
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!
@@ -89,14 +89,38 @@ type CardList = Record<string, Card>; | |||
/** Issue new card flow steps */ | |||
type IssueNewCardStep = ValueOf<typeof CONST.EXPENSIFY_CARD.STEP>; | |||
|
|||
/** Data required to be sent to issue a new card */ | |||
type IssueNewCardData = { |
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.
We will also need to send the policyID - it can be in a follow up PR too.
✋ 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.0.6-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.6-0 🚀
|
🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 9.0.6-1 🚀
|
🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 9.0.6-1 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.6-8 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.7-8 🚀
|
const errors = ValidationUtils.getFieldRequiredErrors(values, [INPUT_IDS.LIMIT]); | ||
|
||
// We only want integers to be sent as the limit | ||
if (!Number(values.limit) || !Number.isInteger(Number(values.limit))) { |
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.
FYI, this caused the following issue: #50788. We need separate error messages for these cases
ListItem={RadioListItem} | ||
onSelectRow={({value}) => setTypeSelected(value)} | ||
sections={[{data}]} | ||
shouldDebounceRowSelect |
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.
we should had passed isAlternateTextMultilineSupported
here to allow multi-line description. this missed prop caused #50959
Details
There are still some things that need to be made in a follow-up PR, ideally when the BE is ready.
Fixed Issues
$ #44309
PROPOSAL:
Tests
PREREQUISITES: Be sure that you have access to all betas or 'workspaceFeeds' beta
Offline tests
QA Steps
n/a
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label 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
Screen.Recording.2024-07-03.at.10.10.10.mp4
Android: mWeb Chrome
Screen.Recording.2024-07-03.at.10.18.18.mp4
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-07-03.at.09.51.50.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-07-03.at.10.13.27.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-07-03.at.08.48.37.mp4
MacOS: Desktop
Screen.Recording.2024-07-03.at.09.16.05.mp4