-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[$250] Remove MoneyRequestDatePage.js and copy any changes since Nov 27 into IOURequestStepDate.js #34608
Comments
Triggered auto assignment to @muttmuure ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Remove MoneyRequestDatePage.js and copy any changes since Nov 27 into IOURequestStepDate.js What is the root cause of that problem?Component update What changes do you think we should make in order to solve the problem?I checked all the commits after 27th Nov and the new component is updated according to the old component, we just need to remove the file and route created for it. I checked the commits thoroughly but will check again in the PR. There are total 7 commits after 27th Nov in the old component and most of them are about renaming constants and Component to remove:
We also need to remove all traces from App/src/libs/Navigation/linkingConfig.ts Line 430 in a2f5bd5
App/src/libs/Navigation/types.ts Line 198 in a2f5bd5
Note: The below 2 steps can be omitted because we only use
Result |
I would love to take this! |
Job added to Upwork: https://www.upwork.com/jobs/~0155d041c50cef598e |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane ( |
I want to work on it |
Would love to work on this |
ProposalPlease re-state the problem that we are trying to solve in this issue.Remove MoneyRequestDatePage.js and copy any changes since Nov 27 into IOURequestStepDate.js What is the root cause of that problem?The main changes are in the App/src/pages/iou/MoneyRequestDatePage.js Lines 103 to 105 in 3033f99
What changes do you think we should make in order to solve the problem?In the new component i.e. here https://github.com/Expensify/App/blob/main/src/pages/iou/request/IOURequestStartPage.js, we will add |
ProposalPlease re-state the problem that we are trying to solve in this issue.Remove MoneyRequestDatePage.js and copy any changes since Nov 27 into IOURequestStepDate.js What is the root cause of that problem?This is a refactor What changes do you think we should make in order to solve the problem?
We have had some changes since Nov 27, 2023 but it's the refactor change and has already been applied in
We only need to remove
Lines 339 to 340 in a8acf44
To handle And when we click on the date option in What alternative solutions did you explore? (Optional)NA |
Upwork job price has been updated to $250 |
Proposal UpdatedAdded a note about why should we not include the action param. |
@muttmuure I won't be able to review proposals before weekend. Please feel free to reassign the issue if it is urgent. |
@muttmuure I have context with this Epic in here. I am happy to take this one as a C+ contributor |
Updated proposal to remove |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
ProposalPlease re-state the problem that we are trying to solve in this issue.This belongs to Wave 5 cleanup What is the root cause of that problem?Replace What changes do you think we should make in order to solve the problem?
Note: At the PR we double check all What alternative solutions did you explore? (Optional)N/A |
@sobitneupane, @muttmuure Huh... This is 4 days overdue. Who can take care of this? |
Triggered auto assignment to @tgolen, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
thank you @DylanDylann, It looks like you copied the link to my proposal wrong hehe Anyway, I'm going to start working on the PR for review |
Thanks. Updated 😄 |
@brunovjk Please wait to be assigned official before implementing PR |
Nice! Thanks!!! |
📣 @brunovjk 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@brunovjk I think these changes also need to be included in your PR:
|
Im on it @tgolen, thank you. PR will be ready for review by 07/02 |
@brunovjk Please raise a PR soon |
@DylanDylann I'm working on it right now, but I'm having a problem with 'edit/split'. In your PRs ( #35641 and #35137) I saw that you 'hard coded' the 'ioutype' in
I thought that we needed to pass a dynamic
|
As I know |
Awesome! Thank you :D |
@DylanDylann The PR is ready for review, but I found a bug, also reproduced on 'staging' that I believe needs to be resolved so that the 'date' flow is perfect, I didn't find a related issue. In PR I bring more information. |
@muttmuure The PR was deployed to production 5 days ago. It seems the automation bot doesn't work on this issue. Please help to move forward this one |
@muttmuure 👉👈 |
All paid up |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
This is a part of #29107. You can look at that issue for more context behind the cleanup process.
Problem
The app has two redundant components:
Old Component:
MoneyRequestDatePage
New Component
IOURequestStepDate
Solution
Following the examples (example 1, example 2), the Old Component needs to be completely removed from the codebase
:action
param (instead of being hard-coded with"create"
)isEditing
to use the new action param from the routeUpwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: