Open cretadn22 opened 2 weeks ago
@ikevin127 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]
@cretadn22 Regarding the PR description tests, I'd be more specific so the QA team has a better understanding of the changes and what to test.
Note: Here's the pastebin so you can easily copy & paste the below steps in your PR's description.
[!tip]
Test 1
Precondition: User does NOT have betas enabled, hence no Rate field will be displayed in Distance confirmation page.
- Log in and go to Collect workspace settings.
- Enable and go to Distance rates.
- Click [+ Add rate].
- Add a new rate and make sure it's Enabled.
- Disable previously existing rate (default $0.670 / mile).
- Go to workspace chat.
- Start submitting a Distance request.
- On Distance confirmation page, verify that the enabled rate is set as default in the Distance field (see below).
Test 2
Precondition: User does has betas enabled, hence the Rate field will be displayed in Distance confirmation page.
- Log in and go to Collect workspace settings.
- Enable and go to Distance rates.
- Click [+ Add rate].
- Add a new rate and make sure it's Enabled.
- Disable previously existing rate (default $0.670 / mile).
- Go to workspace chat.
- Start submitting a Distance request.
- On Distance confirmation page, verify that the enabled rate is set as default in the Rate field (see [1.]).
- On Distance confirmation page, click on the Rate field and verify that the disabled rate is not displayed (see [2.]).
1 | 2 |
---|---|
### 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.@cretadn22 I will ✅ Approve once the 2 comments (https://github.com/Expensify/App/pull/42330#discussion_r1619474285 and https://github.com/Expensify/App/pull/42330#issuecomment-2138292122) are addressed.
Note: Please don't force-push anymore as this goes against the guidelines, see Submit your pull request for final review:
- Please never force push when a PR review has already started (because this messes with the PR review history)
@ikevin127 Thanks for your comments, It will greatly assist me in future PRs.
@neil-marcellini Your comments make sense, I made an update to address your suggestion
@neil-marcellini I addressed your comment and removed the description of getMileageRates function
$ https://github.com/Expensify/App/issues/42284 PROPOSAL: https://github.com/Expensify/App/issues/42284#issuecomment-2116574341
Tests
Offline tests
QA Steps
Test 1
Precondition: User does NOT have betas enabled, hence no Rate field will be displayed in Distance confirmation page.
Test 2
Precondition: User does has betas enabled, hence the Rate field will be displayed in Distance confirmation page.
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/168617111/86dba335-3b6a-4293-9617-e49bdf02c632Android: mWeb Chrome
https://github.com/Expensify/App/assets/168617111/36a647d9-0fa9-4524-8c08-6f5f0771a622iOS: Native
https://github.com/Expensify/App/assets/168617111/8abe2dc5-ca80-4cda-99b8-b57c93fde658iOS: mWeb Safari
https://github.com/Expensify/App/assets/168617111/0b00bf67-5173-433c-b735-3256d9921385MacOS: Chrome / Safari
https://github.com/Expensify/App/assets/168617111/eb1ae904-efb9-4299-be5f-8628d96398c4MacOS: Desktop
https://github.com/Expensify/App/assets/168617111/5aa54f6e-7834-4964-afa5-a2c12f69b767