Skip to content
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] Migrate BaseOptionsSelector.js to function component #16184

Closed
1 task
marcaaron opened this issue Mar 20, 2023 · 29 comments
Closed
1 task

[$250] Migrate BaseOptionsSelector.js to function component #16184

marcaaron opened this issue Mar 20, 2023 · 29 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2

Comments

@marcaaron
Copy link
Contributor

marcaaron commented Mar 20, 2023

Class Component Migration

Filenames

Task

  • We currently have some class components in our codebase that we would like to refactor to a function component.
  • Here's a link with some general advice on how to refactor a class component to a function component: https://react.dev/reference/react/Component#alternatives
  • If you need additional guidance, please ask in #expensify-open-source
  • Test for any regressions and verify that there are no breaking changes
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018a6f55da7cfbd26a
  • Upwork Job ID: 1727071636393496576
  • Last Price Increase: 2023-11-21
@marcaaron marcaaron added Engineering Improvement Item broken or needs improvement. labels Mar 20, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Mar 20, 2023
@Expensify Expensify unlocked this conversation Mar 21, 2023
@marcaaron marcaaron changed the title [HOLD] Migrate BaseOptionsSelector.js to function component [HOLD][$250] Migrate BaseOptionsSelector.js to function component Apr 13, 2023
@s-alves10
Copy link
Contributor

I'd love to work on this.

@dayana7204
Copy link
Contributor

I am ready to work on this ticket. :)

@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

📣 @dayana7204! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@olexyt
Copy link
Contributor

olexyt commented Jul 7, 2023

I can work on this issue.

@rayane-djouah
Copy link
Contributor

Can I work on this issue?

@mejed-alkoutaini
Copy link

I'm able to work on this if needed!

@mkhutornyi
Copy link
Contributor

I'd like to work on this.

@KrAbhas
Copy link
Contributor

KrAbhas commented Jul 19, 2023

I can work on this

@dexexpert
Copy link

I hope to work on this task.

@mountiny mountiny changed the title [HOLD][$250] Migrate BaseOptionsSelector.js to function component [$250] Migrate BaseOptionsSelector.js to function component Sep 5, 2023
@Piotrfj
Copy link
Contributor

Piotrfj commented Sep 5, 2023

Hi I'm Piotr from Callstack - expert contributor group - will start investigation in this area

@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2023

📣 @Piotrfj! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@Piotrfj
Copy link
Contributor

Piotrfj commented Sep 11, 2023

Today im OOO

@Piotrfj
Copy link
Contributor

Piotrfj commented Nov 21, 2023

PR is under internal review now

@thienlnam thienlnam added the External Added to denote the issue can be worked on by a contributor label Nov 21, 2023
Copy link

melvin-bot bot commented Nov 21, 2023

Job added to Upwork: https://www.upwork.com/jobs/~018a6f55da7cfbd26a

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 21, 2023
Copy link

melvin-bot bot commented Nov 21, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @shubham1206agra (External)

@melvin-bot melvin-bot bot added the Daily KSv2 label Nov 21, 2023
@thienlnam thienlnam removed Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Nov 21, 2023
@thienlnam thienlnam self-assigned this Nov 21, 2023
@thienlnam
Copy link
Contributor

Just added the External label to get a C+ assigned to this issue. The PR is ready to review here

cc @shubham1206agra

@shubham1206agra
Copy link
Contributor

@thienlnam Looks like checklist is not complete. Should I wait for review?

@thienlnam
Copy link
Contributor

Ah good point, let's have the contributor complete that first

@Piotrfj
Copy link
Contributor

Piotrfj commented Dec 5, 2023

Link to PR: #31606

Copy link

melvin-bot bot commented Feb 27, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

Copy link

melvin-bot bot commented Feb 27, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

Copy link

melvin-bot bot commented Feb 27, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

Copy link

melvin-bot bot commented Feb 27, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

@roryabraham roryabraham self-assigned this Feb 28, 2024
@roryabraham
Copy link
Contributor

I think we're going to revert the PR because it caused many regressions. That said, I did a substantial amount of refactoring to get this component to a much better place in #37353. @Piotrfj I hope you use it as a reference to help clean up this component. and I want to review the PR as well

@roryabraham roryabraham added the Weekly KSv2 label Feb 28, 2024
@roryabraham
Copy link
Contributor

Created a slack thread to discuss: https://expensify.slack.com/archives/C01GTK53T8Q/p1709156803359359

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Feb 28, 2024
@Piotrfj
Copy link
Contributor

Piotrfj commented Feb 29, 2024

Update: re-refactoring is ready to be tested, it fixes all the regression issues I was aware of. Please test it (especially the tags, because I could not yet replicate the scenario on my site).
The fix is based on the @roryabraham solution, so special thanks for him!
Link to PR: #37494

@roryabraham
Copy link
Contributor

Sorry for the back-and-forth, but coming from this discussion I see that OptionsSelector is essentially deprecated, so I propose that we actually close the above PR and this issue, and plan to focus on removing the deprecated OptionsSelector altogether

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Mar 1, 2024
@Piotrfj
Copy link
Contributor

Piotrfj commented Mar 7, 2024

@roryabraham let me know about the final decision about this issue

@roryabraham
Copy link
Contributor

Yes, let's not invest time and money refactoring something that's deprecated. Let's instead invest resources in removing it completely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests