-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 Assignment Flow #48278
[No QA] Card Assignment Flow #48278
Conversation
@allroundexperts 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 good catch! thanks! Fixed now 🙂 |
Lint failing @koko57 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-09-03.at.7.59.51.PM.movAndroid: mWeb ChromeScreen.Recording.2024-09-03.at.8.07.22.PM.moviOS: NativeScreen.Recording.2024-09-03.at.7.53.31.PM.moviOS: mWeb SafariScreen.Recording.2024-09-03.at.7.58.02.PM.movMacOS: Chrome / SafariScreen.Recording.2024-09-03.at.7.31.05.PM.movMacOS: DesktopScreen.Recording.2024-09-03.at.7.44.16.PM.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.
Functionality wise, this LGTM, Tests well on all platforms, left some comments above, I will approve the PR after those are addressed @koko57
@koko57 , this has conflicts :) |
@allgandalf thanks! I'll be starting my workday shortly, I'm taking care of this PR first |
Co-authored-by: Gandalf <whogandalf@gmail.com>
@allgandalf sorry it's taking so long but the DatePicker is styled strangely and with FormWrapper styles gives a mix that is kinda hard to solve. I will look into the other places that DatePicker is used if the submit button is also displayed this way. The rest of the comments are resolved/answered. |
Thanks for informing @koko57 , please let me know, if it is so difficult, we can remove the button and go to next page when an user will click on the date? is that possible? (Also, typecheck if failing, please have a look) |
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 should pass the typecheck
src/languages/en.ts
Outdated
@@ -2792,6 +2793,24 @@ export default { | |||
assignCard: 'Assign card', | |||
cardNumber: 'Card number', | |||
customFeed: 'Custom feed', | |||
assignCard: 'Assign card', |
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.
assignCard: 'Assign card', |
src/languages/es.ts
Outdated
@@ -2839,6 +2840,24 @@ export default { | |||
assignCard: 'Asignar tarjeta', | |||
cardNumber: 'Número de la tarjeta', | |||
customFeed: 'Fuente personalizada', | |||
assignCard: 'Asignar tarjeta', |
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.
assignCard: 'Asignar tarjeta', |
@allgandalf I've just pushed a typecheck fix the very moment you commented 😃 For the style problem I've also just found the culprit should be not so complicated to fix this |
@allgandalf - requesting rereview |
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.
Lets get this shipped 🚀
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.
Nice!
✋ 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/yuwenmemon in version: 9.0.30-0 🚀
|
@@ -0,0 +1,92 @@ | |||
import React from 'react'; |
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 missed to implement the cardName page from the design doc, this caused #51908.
Note to me: @allgandalf please be more careful and read the design doc thoroughly 😅
<ScreenWrapper | ||
style={styles.pb0} | ||
includePaddingTop={false} | ||
includeSafeAreaPaddingBottom={false} |
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.
@Ollyws i think the recent changes in the navigation stack / the one where folks from malgero updated the android view to occupy the full length of the screen has caused this regression and not this PR , can you check once ?
|
||
const [dateOptionSelected, setDateOptionSelected] = useState(data?.dateOption ?? CONST.COMPANY_CARD.TRANSACTION_START_DATE_OPTIONS.FROM_BEGINNING); | ||
const [isModalOpened, setIsModalOpened] = useState(false); | ||
const [startDate, setStartDate] = useState(DateUtils.extractDate(new Date().toString())); |
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 have used the default start date from the data
variable. Not doing this caused #55960
/> | ||
<FormAlertWithSubmitButton |
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 is no space between the list and the button, and we add it here #57364.
Details
Fixed Issues
$ #47377
PROPOSAL: -
Tests
PREREQUISITES: Workspace Feed / all betas enabled
to test on desktop: open devtools and type
window.location = "https://dev.new.expensify.com:8083/settings/workspaces/[workspaceID]/company-cards/[feed]/assign-card"
on mobile: (with simulator opened) type in terminal
npx uri-scheme open new-expensify://settings/workspaces/[workspaceID]/company-cards/[feed]/assign-card --ios
ornpx uri-scheme open new-expensify://settings/workspaces/[workspaceID]/company-cards/[feed]/assign-card --android
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
Screen.Recording.2024-09-02.at.18.20.36.mp4
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-02.at.18.05.26.mp4
iOS: mWeb Safari
https://github.com/user-attachments/assets/de28f015-aa57-45d0-9732-717f6dd3238b

MacOS: Chrome / Safari
Screen.Recording.2024-08-29.at.17.17.10.mp4
MacOS: Desktop
https://github.com/user-attachments/assets/3383419e-1e4a-4937-8ca9-02b54fdc4826
