Closed Tony-MK closed 3 days ago
@hungvu193 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]
Can you address the linting? @Tony-MK
Do you think we also should to revert the amount when we clicked the above error?
https://github.com/user-attachments/assets/fbef07d4-9dad-41f8-b606-b8a215035dff
Can you address the linting? @Tony-MK
@hungvu193, Seems like the linting issue is not due to my changes but because of rulesdir/no-default-id-values
.
You can read this slack thread for more info.
Can you address the linting? @Tony-MK
@hungvu193, Seems like the linting issue is not due to my changes but because of
rulesdir/no-default-id-values
.You can read this slack thread for more info.
I know that. But I think you mentioned wrong slack thread. Anyway, we still need to address the linting
Do you think we also should to revert the amount when we clicked the above error?
https://github.com/user-attachments/assets/fbef07d4-9dad-41f8-b606-b8a215035dff
How about this one?
Can you address the linting? @Tony-MK
@hungvu193, Seems like the linting issue is not due to my changes but because of
rulesdir/no-default-id-values
. You can read this slack thread for more info.I know that. But I think you mentioned wrong slack thread. Anyway, we still need to address the lining
I just wanted to point out the two first lines.
[!NOTE] Hey team, please read this section thoroughly and ensure you follow it in the PRs you write/review. The main purpose of this eslint change was to ensure we gradually start handling missing IDs properly.
Yeah, we can address the lining, but won't it cause merge conflicts with other PRs trying to fix the same lint issue?
Do you think we also should to revert the amount when we clicked the above error?
How about this one?
Frankly, I also don't know about this one.
I have considered it and would like your opinion because I originally thought it was only the last error.
What do you think?
Yeah, we can address the lining, but won't it cause merge conflicts with other PRs trying to fix the same lint issue?
No worries, I think that's simple change so it won't be a problem.
Frankly, I also don't know about this one.
I have considered it and would like your opinion because I originally thought it was only the last error.
Yep. Agree.
Regarding this comment from mkzie2 I don't think I can reproduce the bug that he mentioned. Can you confirm? @Tony-MK. ty
Regarding this comment from mkzie2 I don't think I can reproduce the bug that he mentioned. Can you confirm? @Tony-MK. ty
I also could not reproduce it after attempting a couple of times.
Cool. Ty
Can you fix lint again ๐ ?
lint is still failing..
Hey @hungvu193, I have edited MoneyRequestView
to pass the lint, but PureReportActionItem
is the only file left. I think we should allow it since it's long, hence it has a lot of functions to change. Considering some variables are named exactly as the function name, it becomes tiresome. Also, the last commit on PureReportActionItem
didn't pass the lint because of one specific rule. If it's a must, can we do it in a separate PR?
Considering some variables are named exactly as the function name, it
with the function/variable that have the same name, you can change its alias like this:
import {functionName as functionNameUtil} from '..'
you can reference here: https://expensify.slack.com/archives/C01GTK53T8Q/p1736838528732529
Please keep in mind that we can't merge the PR without all the check passing, we will always need to address the lint anyway.
Okay, on it. It should be done by the end of the day.
Thanks for the updates @Tony-MK and your effort on the lint fixes. I appreciate it ๐
### 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/tylerkaraszewski in version: 9.0.89-0 ๐
platform | result |
---|---|
๐ค android ๐ค | success โ |
๐ฅ desktop ๐ฅ | success โ |
๐ iOS ๐ | success โ |
๐ธ web ๐ธ | success โ |
๐ค๐ android HybridApp ๐ค๐ | success โ |
๐๐ iOS HybridApp ๐๐ | success โ |
Explanation of Change
After the error message is cleared, change the transaction amount and currency to the values used to pay.
Fixed Issues
$ https://github.com/Expensify/App/issues/54471 PROPOSAL: https://github.com/Expensify/App/issues/54471#issuecomment-2560220812
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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 - Native.webm](https://github.com/user-attachments/assets/43892741-2c99-4045-b511-a9b3426e4597)Android: mWeb Chrome
[Android - mWeb.webm](https://github.com/user-attachments/assets/9e5beb02-63f4-4e5d-bc9c-5898f7a9ff5d)iOS: Native
https://github.com/user-attachments/assets/37c758fd-8622-4a8c-a320-24eae5608aa0iOS: mWeb Safari
https://github.com/user-attachments/assets/3674c37e-374c-4e23-8c56-1cb36b45bd0bMacOS: Chrome / Safari
https://github.com/user-attachments/assets/b4f1c46b-fbee-41d4-980c-560d6a4ffc2dMacOS: Desktop
https://github.com/user-attachments/assets/ad6a8d63-f499-4bfa-81f5-1182eb72a16e