App: [HOLD for payment 2024-05-02] [$250] Remove `MoneyRequestConfirmationList.js` and copy any changes since Nov 27 into `MoneyTemporaryForRefactorRequestConfirmationList.js`
This is a part of https://github.com/Expensify/App/issues/29107. You can look at that issue for more context behind the cleanup process.
Problem
The app has two redundant components:
Old Component: MoneyRequestConfirmationList
New Component MoneyTemporaryForRefactorRequestConfirmationList
Solution
Now that most of the referenced components have been cleaned up and following the examples (example 1, example 2), the Old Component needs to be completely removed from the codebase
- Look at the history of the Old Component
- If there are any changes since Nov 27, 2023 which have not been added to the New Component, copy those changes
- Replace all uses of the Old Component with the New Component
- Remove all traces of Old Component
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~01a11e5428841cbb31
- Upwork Job ID: 1775636536127291392
- Last Price Increase: 2024-04-03
- Automatic offers:
- eh2077 | Reviewer | 0
- brunovjk | Contributor | 0
Issue Owner
Current Issue Owner: @bfitzexpensifyAbout this issue
- Original URL
- State: open
- Created 3 months ago
- Comments: 40 (28 by maintainers)
@brunovjk Hey, I’ve took over this issue and the last thing to do is to replace
<OptionsSelector/>component insideMoneyRequestConfrimation...component with<SelectionList/>. When do you think your PR will get merged? I assume it’s close 😃We already have a PR https://github.com/Expensify/App/pull/40659 to fix the DB
@brunovjk Sure. Please tag me when the PR is ready, thank you!
At first glance it seems to work 👍 It’s probably worth for people who reviewed the PR to have a look at it too.
@brunovjk I think something might be off when it comes to displaying footer. I can’t review the Split expense request
I might be mistaken but this condition should probably be reversed - we should display footer when editinghttps://github.com/Expensify/App/blob/9b2c5187e2203d83bba50bfa66936691788cbd18/src/components/MoneyRequestConfirmationList.tsx#L931Edit: It’s not as simple as reversing condition
We merged @MrMuzyk.
Yep! We are good to go 😄
TS migration has been merged. I will start working on our PR immediately.
@brunovjk I think generally you can start working on the PR once the migration PR is merged.
Yeah, I agree it’s better to do them one by one. I found there’s an ongoing PR https://github.com/Expensify/App/pull/37181 to migrate
MoneyTemporaryForRefactorRequestConfirmationList.js. I think we can wait for that PR.Once the TS migration PR is merged, I think we can let @brunovjk to take this task as their proposal first pointed out we should replace
MoneyConfirmationListwithMoneyTemporaryForRefactorRequestConfirmationListand @brunovjk also started the discussion about TS migration.@tgolen All yours.
🎀👀🎀 C+ reviewed
Sorry everyone, I confused
IOURequestStepConfrimation.jswithMoneyTemporaryForRefactorRequestConfirmationList.js.This issue doesn’t need to have anything major done with
IOURequestStepConfirmation.js.This issue should be for replacing
MoneyConfirmationListwithMoneyTemporaryForRefactorRequestConfirmationList.Ah, I would probably avoid doing both the TS migration and the cleanup in the same task (to keep the PR scope smaller). Would it be better to do the TS migration first or second?
@brunovjk Thanks for the update. That’s a good point.
@tgolen Should we do migrating
MoneyTemporaryForRefactorRequestConfirmationListto TS in this task? Or in a separate task just like other TS migration tasks?