Closed hungvu193 closed 1 week ago
This should be blocked for Invoice Account Selector PR as well right? I can see changes that I just reviewed.
This PR is not ready yet. Please wait until I mark it as ready π
Ohh π. I stumbled upon this one.
Hold for #41554
Still being held but almost ready!
@mananjadhav 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]
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and not onIconClick
).myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components using Avatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. for onClick={this.submit}
the method this.submit
should be bound to this
in the constructor)this
are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this);
if this.submit
is never passed to a component event handler like onClick
)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG
)Avatar
is modified, I verified that Avatar
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 the Test
steps.We did not find an internal engineer to review this PR, trying to assign a random engineer to #39742 as well as to this PR... Please reach out for help on Slack if no one gets assigned!
Tagging @Expensify/design. I wanted to also confirm if it's the only email in the list do we need to navigate twice?
Thanks for bringing this upβno, we do not want to do the double-navigate thing you're showing. The second screen should more closely follow what we do for language select (for example). That would give us something like this:
Where we have the push input on the first screen, and then the immediate screen after that is the list with the explainer text.
Thanks for bringing this upβno, we do not want to do the double-navigate thing you're showing. The second screen should more closely follow what we do for language select (for example). That would give us something like this:
Where we have the push input on the first screen, and then the immediate screen after that is the list with the explainer text.
Huge +1.
Exactly. Thanks for clarifying, @dannymcclain . I assume this is reflected in the Figma file? If not, let's update it
I did it like the docs: https://docs.google.com/document/d/1VEIzxwNzedLNestAFVT5InGvD4N6FHb0922xjMGOkko/edit#heading=h.fbu7ml85rrhl Can we update the docs?
Big agree with where you landed Danny.
Cool. Let's me update it then.
cc @Expensify/design does that look good to you too now?
That looks right to me!
@lakchote @hungvu193 I've raised another question related to pendingFields.
@lakchote This will be ready for your final review π
@lakchote This will be ready for your final review π
Approved, conflicts to be solved and then I'll merge it @hungvu193 π
Cool! I've resolved the conflicts
I've fixed the lint as well, this will be ready to merge π
π Deployed to staging by https://github.com/lakchote in version: 1.4.74-0 π
platform | result |
---|---|
π€ android π€ | success β |
π₯ desktop π₯ | success β |
π iOS π | success β |
πΈ web πΈ | success β |
Details
Create the Preferred exporter select page
Fixed Issues
$ https://github.com/Expensify/App/issues/39742 PROPOSAL: N/A
Tests
Precondition: User connected to Xero
Offline tests
Same as Tests.
QA Steps
Same as Tests.
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
https://github.com/Expensify/App/assets/16502320/11c5eb54-f44a-4402-afc5-6ea0f21f7378Android: mWeb Chrome
https://github.com/Expensify/App/assets/16502320/12693b32-8eda-4cf6-b543-28c42465f0e4iOS: Native
https://github.com/Expensify/App/assets/16502320/47a87a0d-85bf-463b-89a2-323fd1bec20fiOS: mWeb Safari
https://github.com/Expensify/App/assets/16502320/580d7ade-952c-46b9-8726-c746b59f6b5fMacOS: Chrome / Safari
https://github.com/Expensify/App/assets/16502320/d12046d5-4c1e-4e91-b870-d7be9e28ad8fMacOS: Desktop
https://github.com/Expensify/App/assets/16502320/25796372-227c-4194-a48a-325ee78f5aa3