-
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] Card settings page #45415
[No QA] Card settings page #45415
Conversation
@allgandalf 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] |
Co-authored-by: Gandalf <[email protected]>
src/libs/CardUtils.ts
Outdated
/* eslint-disable @typescript-eslint/prefer-nullish-coalescing */ | ||
return Object.values(bankAccountsList).filter((bankAccount) => bankAccount?.accountData?.allowDebit || bankAccount?.accountData?.type === CONST.BANK_ACCOUNT.TYPE.BUSINESS); |
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.
Why not ??
nullish operator here, any specific reason ?
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.
if allowDebit would be false it would return false and wouldn't check for the type. We could have false for allowDebit but type would be fine, but we would get false incorrectly
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.
Could we get around this? I believe that any eslnt rule disable means that react-compiler does not process this file, we should try to avoid adding any eslint-disable to new code
@allgandalf fixed the navigation |
BugOn frequency page, the highlight doesn't move to the newly selected value Screen.Recording.2024-07-19.at.3.44.39.PM.mov |
EDIT: I just checked with other pages, it is the same behaviour we shouldn't do anything here |
@allgandalf - this is something that has not been implemented for this component. @VickyStash added it to https://github.com/Expensify/App/pull/45601/files, we can incorporate these changes in a follow-up (when connecting to the BE) or probably this would work out of the box |
All good then! |
NAB: When i click on learn more nothing happens really, is it for future implementation @koko57 ? |
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.
This tests well on all platforms, the bug i found was mentioned here, would actually be solved when we implement the BE
, I will keep an eye out because that bug is very much a blocker to me if i cannot shift the focus on options, rest assured, we can merge this one before the week closes, thanks for the back and forth @koko57!
NAB: When i click on learn more nothing happens really, is it for future implementation @koko57 ? yep, the doc is being worked on, once it's done we'll get a proper url to link to |
Great! It would be awesome to get this merged today, as it's the last PR left in Phase 2. 🎉 |
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.
Changes look good to me, thank you @koko57 and @allgandalf for quick testing.
This is now all ready for @MariaHCD
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! Just one comment
Co-authored-by: Maria D'Costa <[email protected]>
✋ 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/MariaHCD in version: 9.0.11-0 🚀
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 9.0.11-2 🚀
|
probably just got the ping but looks like the "Reconciliation account" link isn't working properly. Not sure if this is the only issue but I am not sure it's accounting for if you don't have a reconciliation account - or an accounting integration - yet |
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.11-5 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.12-0 🚀
|
Details
Fixed Issues
$ #44311
$ #44312
$ #44313
PROPOSAL: -
Tests
PREREQUISITES: Comment out
AccessOrNotFoundWrapper
inWorkspaceCardSettingsPage
,WorkspaceSettlementAccountPage
andWorkspaceSettlementFrequencyPage
MOBILE: (with simulator open and project build) in the terminal type
npx uri-scheme open new-expensify://settings/workspaces/[workspaceId]/expensify-card/settings --ios
to open on iOS,npx uri-scheme open new-expensify://settings/workspaces/[workspaceId]/expensify-card/settings --android
to open on android.DESKTOP: with the desktop app open, in the browser type https://staging.new.expensify.com/settings/workspaces/[workspaceId]/expensify-card/settings, when the prompt to open the app will be displayed, open the app
🚨 NOTE: for now I'm not filtering out eligible accounts for easier testing. It will be done when integrating with the backend. I also haven't disabled Frequency menu item to make it possible to check both Daily and Monthly state
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop