Closed hungvu193 closed 1 week ago
@getusha 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]
@Expensify/design could you please review the Import
and Advanced
Xero screens?
We should have tagged you beforehand on the related PRs (that's my fault - and I'm sorry for that). Since the external agency working on this was OOO this entire week, C+ contributors have been hard at work to gain a good momentum to comply with the external commit in June.
But still, let's not sacrifice quality for speed. Quality is our differentiator ultimately, and so we need your expert eyes on this!
Thank you
We did not find an internal engineer to review this PR, trying to assign a random engineer to #41560 as well as to this PR... Please reach out for help on Slack if no one gets assigned!
### 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.@hungvu193 please fix lint
@hungvu193 please fix lint
Fixed!
I don't think "Other integrations" is supposed to be bold:
This is what we have in Figma and the design doc:
cc @lakchote - let me know the best place to address my comment above.
cc @lakchote - let me know the best place to address my comment above.
Thanks @shawnborton, here would be the place to centralize your reviews/requested changes for the Import
, Advanced
, and main Xero Accounting
screens.
So, you're in the right place.
Also looks like the organization name is bold:
But it shouldn't be based on the Figma/design doc screens:
Otherwise I think it looks pretty good to me.
@hungvu193 could you please make the above changes ^
I know they aren't related to this PR, so you can open a new issue or make them here if they're quick
@hungvu193 could you please make the above changes ^
I know they aren't related to this PR, so you can open a new issue or make them here if they're quick
Np, I updated it.
:hand: This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.
@lakchote just to confirm, where will you be addressing the feedback I posted above? Thanks!
@lakchote just to confirm, where will you be addressing the feedback I posted above? Thanks!
It looks like it was addressed (the Xero organization should not be bold), here.
However by giving it a second look, the Other organisations
modification (it shouldn't be bold too) slip away and I apologize for that.
I'm going to address this in my PR here, thank you!
Sounds good, please tag me for review over there. Do you know which PR introduced the breaking change (going from regular weight to bold) in the first place?
Sounds good, please tag me for review over there. Do you know which PR introduced the breaking change (going from regular weight to bold) in the first place?
It comes from this PR, I've proceeded with the merge and I should have tagged you. We were in a rush as this PR blocked every other single PR related to Xero, still we should follow the normal process (it's now printed in my brain, don't worry π).
π Deployed to production by https://github.com/marcaaron in version: 1.4.71-6 π
platform | result |
---|---|
π€ android π€ | success β |
π₯ desktop π₯ | success β |
π iOS π | success β |
πΈ web πΈ | success β |
Details
Fix
ConnectionLayout
padding issueFixed Issues
$ https://github.com/Expensify/App/issues/41560 PROPOSAL: N/A
Tests
ConnectionLayout
component (ie: XeroAdvancePage, XeroCustomerConfigurationPage)Offline tests
N/A
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
https://github.com/Expensify/App/assets/16502320/639a351d-883d-4385-9d2f-ba7c7a78a889MacOS: Desktop