-
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] Create new Edit Card Name page #45473
[No QA] Create new Edit Card Name page #45473
Conversation
@jayeshmangwani 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] |
I can work on this review :)) |
src/pages/workspace/expensifyCard/WorkspaceEditCardNamePage.tsx
Outdated
Show resolved
Hide resolved
Reviewer Checklist
Screenshots/Videos |
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.
Basic functionalities like save button
coming up with keyboard, offline indicator not present on large screens etc, for a form input are complete and work well.
I have overlooked the validation part as that would require the BE
to be implemented first as we need to know what the backend accepts and what doesn't. Overall simple page nothing much technically, the design team too approved this over the issue.
LGTM and tests well on all platforms!
@mountiny please, take a look at the PR 🙂 |
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, all yours @MariaHCD
I think in terms of name, we do not have any validation except duplicated names for now. ie you cannot have the same name for the card on the feed
umm, i think we can have a follow up to this validation whenever the |
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, LGTM! For now, we have a validation of 255 characters when creating a card (API command CreateExpensifyCard
). But we can add validation later as well.
✋ 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.10-2 🚀
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.0.10-3 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.10-7 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.11-5 🚀
|
Details
Create new Edit Card Name page.
Fixed Issues
$ #44327
PROPOSAL: N/A
Tests
Pre-step:
Comment out the feature check in the WorkspaceEditCardNamePage file, so you can access the screen.
Steps:
/expensify-card/1/edit/name
, for example:https://dev.new.expensify.com:8082/settings/workspaces/YOUR_POLICY_ID/expensify-card/1/edit/name
.Offline tests
N/A
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 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