Closed rushatgabhane closed 1 week ago
This has broken spanish language, and we'll need a spanish copy for all text here -
Code changes look fine. I’ll finish the checklist in a few hours.
@mollfpr 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]
This has broken spanish language, and we'll need a spanish copy for all text here -
Looks like our casing might be off on a couple of labels here for Purchase bill date
and Xero bank account
, as well as the out of pocket export value to be Purchase bill
. I'd expect the first letter to be capitalised only, like the others. Same goes for the educational subtext. We don't need to capitalise Purchase Bill
in that paragraph, like we don't Xero bank account
in the second educational subtext for company cards.
Is that value in the push input just a placeholder? If not, it should be Date of last expense
.
Discussion point: I think the text we're using here feeds confusion and we shouldn't carry it over from OldDot. There's a setting in Advanced
to change the status of the purchase bill on export to Awaiting payment
, Awaiting approval
or Draft
. So for example, if I change that to Draft
the expense report will export as a draft purchase bill, but the subtext we're using here doesn't reflect that. The same goes for the purchase bill date, if I change that to Exported date
for example it doesn't reflect that either, needless to say that phrasing is ambiguous as to whether it's the last day of the month or the date of last expense in that month (both bolded for emphasis below):
Reports will export as a Purchase Bill awaiting payment, posting on the last day of the month in which expenses were incurred. This is the only export option with Xero.
We could make that text "dynamic", but we could also avoid it and make this clearer. I don't really see why we need to have the purchase bill status in a separate place in Advanced
, so I think we could tighten this up by doing the following:
Export
That would get us this:
Thoughts, @Expensify/design @zanyrenney?
Is that value in the push input just a placeholder? If not, it should be Date of last expense
Yes, it'll be taken care in another issue that implements the push input page
Added english copy from the docs
I agree that we need to refine it
Yeah, I still think it's incorrect. 😄
I like your suggestions Tom!
Dope, appreciate the hipcheck! @rushatgabhane can you make those changes then, please?
@trjExpensify
Does this look better? I moved set status from advanced to export page. Please ignore the status and date values.
@trjExpensify
Does this look better? I moved set status from advanced to export page. Please ignore the status and date values.
Much better! Small tweak to make that sentence flow better:
Reports will export as a purchase bill, using the date and status you select below.
@trjExpensify While you're at the content, can we also get the Spanish translations?
Just this line?
Reports will export as a purchase bill, using the date and status you select below.
Waiting on the Spanish translation before I get to the checklist. Tested on web so far and all looks good.
Just this line?
No, the whole page actually. @trjExpensify we need translation for this entire page 🙇
Asked on slack - https://expensify.slack.com/archives/C036QM0SLJK/p1714903467537949
Just this line?
No, the whole page actually. @trjExpensify we need translation for this entire page 🙇
Asked on slack - https://expensify.slack.com/archives/C036QM0SLJK/p1714903467537949
Asked for spanish translations here
Also, friendly reminder to add QA and tests to the description
@Expensify/design could you double check everything look correct?
Where is the best place to find the latest screenshots?
Starting to test now.
### 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 #39735 as well as to this PR... Please reach out for help on Slack if no one gets assigned!
@lakchote @pecanoro All yours.
Also, friendly reminder to add QA and tests to the description
@rushatgabhane Can you update the QA steps section to atleast mention same as Tests
?
The QA steps are super short, I am assuming that besides the page opening, we need to test the different options work?
The QA steps are super short, I am assuming that besides the page opening, we need to test the different options work?
@pecanoro no, we don't need to test if the options work. They will be implemented in separate PRs
@mananjadhav We are missing one checkbox in the checklist 😄
@pecanoro Just done.
### 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.: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.
🚀 Deployed to staging by https://github.com/lakchote in version: 1.4.72-0 🚀
platform | result |
---|---|
🤖 android 🤖 | success ✅ |
🖥 desktop 🖥 | success ✅ |
🍎 iOS 🍎 | success ✅ |
🕸 web 🕸 | success ✅ |
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.72-1 🚀
platform | result |
---|---|
🤖 android 🤖 | success ✅ |
🖥 desktop 🖥 | success ✅ |
🍎 iOS 🍎 | success ✅ |
🕸 web 🕸 | success ✅ |
Details
Fixed Issues
$ https://github.com/Expensify/App/issues/39735 PROPOSAL:
Tests
Pre-requisite:
Accounting
Beta andXero
beta.Test
Offline 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
iOS: Native
https://github.com/Expensify/App/assets/29673073/7009336c-be2e-4b9c-833b-ea9b0c81a50diOS: mWeb Safari
https://github.com/Expensify/App/assets/29673073/6c97b770-38cb-4d6d-b403-c590b0ab9a1bMacOS: Chrome / Safari
MacOS: Desktop