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

[HOLD for payment 2023-12-08] [Theme Switching Migration] Master File Tracking #31677

Closed
1 of 31 tasks
grgia opened this issue Nov 22, 2023 · 33 comments
Closed
1 of 31 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2

Comments

@grgia
Copy link
Contributor

grgia commented Nov 22, 2023

Instructions - #27346

Coming from https://expensify.slack.com/archives/C03UK30EA1Z/p1700644906437179?thread_ts=1700563711.542099&cid=C03UK30EA1Z

We need to migrate the following files to use our styles/theme hooks.

  • src/libs/Navigation/AppNavigator/getRootNavigatorScreenOptions.js
  • src/components/AddressForm.js
  • src/components/Attachments/AttachmentView/AttachmentViewPdf/index.android.js
  • src/components/AvatarWithImagePicker.js
  • src/components/Composer/index.ios.js
  • src/components/Composer/index.android.js
  • src/components/EmojiPicker/EmojiPickerMenu/index.js
  • src/components/Icon/BankIcons.ts
  • src/components/MenuItem.js
  • src/components/ScreenWrapper/index.js
  • src/libs/ComposerUtils/updateNumberOfLines/index.native.ts
  • src/libs/Navigation/AppNavigator/Navigators/CentralPaneNavigator/BaseCentralPaneNavigator.js
  • src/libs/Navigation/AppNavigator/RHPScreenOptions.js
  • src/libs/Navigation/AppNavigator/getRootNavigatorScreenOptions.js
  • src/pages/ReferralDetailsPage.js
  • src/pages/ReimbursementAccount/RequestorStep.js
  • src/pages/settings/Wallet/Card/BaseGetPhysicalCard.js
  • src/pages/settings/Wallet/Card/GetPhysicalCardConfirm.js
  • src/pages/settings/Wallet/Card/GetPhysicalCardName.js
  • src/pages/settings/Wallet/Card/GetPhysicalCardPhone.js
  • src/styles/addOutlineWidth/index.ts

PropTypes

  • src/components/OptionsList/optionsListPropTypes.js
  • src/components/OptionsSelector/optionsSelectorPropTypes.js

Styles

  • src/pages/signin/SignInPageLayout/signInPageStyles/index.native.js
  • src/pages/signin/SignInPageLayout/signInPageStyles/index.js
  • src/styles/containerComposeStyles/index.native.ts
  • src/styles/containerComposeStyles/index.ts
  • src/styles/getContextMenuItemStyles/index.native.ts
  • src/styles/getContextMenuItemStyles/index.ts
  • src/styles/optionRowStyles/index.ts
  • src/styles/optionRowStyles/index.native.ts
@grgia
Copy link
Contributor Author

grgia commented Nov 22, 2023

cc @roryabraham Let me know if you're interested in helping to review these when they're done!

@grgia
Copy link
Contributor Author

grgia commented Nov 22, 2023

cc @chrispader as well

@koko57
Copy link
Contributor

koko57 commented Nov 22, 2023

Could you please assign me to this tracking issue?

@koko57
Copy link
Contributor

koko57 commented Nov 22, 2023

@grgia it turns out we don't need to migrate src/styles/addOutlineWidth/index.ts - it's used in styles/styles.ts

@grgia
Copy link
Contributor Author

grgia commented Nov 22, 2023

@koko57 are you able to update the issue description? If so, feel free to update as needed! I used a strikethrough for that one

@koko57
Copy link
Contributor

koko57 commented Nov 22, 2023

@grgia no, unfortunately I cannot update it 😞

@chrispader
Copy link
Contributor

@grgia should i work on some of this or are these issues/batches already assigned to someone?

@koko57
Copy link
Contributor

koko57 commented Nov 24, 2023

@grgia here's the summary what we've managed to do so far
open PRs:
#31774
#31728
#31723
#31827
#31691 (need to address one comment)

drafts:
#31815

@grgia
Copy link
Contributor Author

grgia commented Nov 27, 2023

@chrispader, we've got PRs up for these; but if you'd like to help review, that would be great!

@koko57
Copy link
Contributor

koko57 commented Nov 27, 2023

@grgia Monday update:
open PRs:
#31774
#31728
#31723
#31827
#31691
#31815

drafts:
#31913
#31831

So now we've got PRs for all the batches 🙂

@grgia
Copy link
Contributor Author

grgia commented Nov 27, 2023

@koko57 amazing to hear! Do the open PRs have the author checklist completed?

@koko57
Copy link
Contributor

koko57 commented Nov 27, 2023

@grgia looks like only this one is missing something, the rest are ok

@koko57
Copy link
Contributor

koko57 commented Nov 28, 2023

Tuesday update:

All the PRs are opened for review 🙂 🎉
I see that some of them are already approved 🙂

@chrispader
Copy link
Contributor

Should we make sure that all of these batches also include changes to propTypes? I noticed some of the components' propTypes haven't been updated, but we might ignore this since we're migrating TypeScript anway?

Wdyt? @grgia

@grgia
Copy link
Contributor Author

grgia commented Nov 29, 2023

@chrispader good point. I think we should include the proptypes for class components until they are officially migrated.

cc @koko57 thoughts on this?

@chrispader
Copy link
Contributor

All the PRs looks good to me! 👍

@grgia
Copy link
Contributor Author

grgia commented Nov 30, 2023

@koko57 @chrispader

It looks like we need to replace default theme import for these files

@koko57
Copy link
Contributor

koko57 commented Nov 30, 2023

@grgia ok I will prepare the PR for the missed themeColors imports inside the components.

Then I can take care of the rest of the files, but please clarify how should I change import like this:
App/src/styles/StyleUtils.ts

@chrispader
Copy link
Contributor

Then I can take care of the rest of the files, but please clarify how should I change import like this: App/src/styles/StyleUtils.ts

We need to pass theme from the component with useTheme to any of the functions that currently depend on themeColors

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Nov 30, 2023
@koko57
Copy link
Contributor

koko57 commented Nov 30, 2023

@chrispader @grgia #32247 ready for review

@chrispader
Copy link
Contributor

@chrispader @grgia - for this file I think it should stay as it is App/src/styles/styles.ts

Yes right. We're removing this import and defaultStyles as soon as all components are migrated.

@grgia
Copy link
Contributor Author

grgia commented Nov 30, 2023

@koko57
I created two issues for these
#32254
#32255

@rezkiy37
Copy link
Contributor

@grgia, I've opened a PR for review - #32250.

@koko57
Copy link
Contributor

koko57 commented Nov 30, 2023

@grgia thanks! we're almost done, but I will link the PRs for this issue. The last PR is nearly there

@grgia
Copy link
Contributor Author

grgia commented Nov 30, 2023

@koko57 @rezkiy37 which PR will include getTooltipStyles.ts?

@koko57
Copy link
Contributor

koko57 commented Nov 30, 2023

@grgia the last one I'm opening in a few minutes - there will be 2 PRs for the Util issue

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 1, 2023
@melvin-bot melvin-bot bot changed the title [Theme Switching Migration] Master File Tracking [HOLD for payment 2023-12-08] [Theme Switching Migration] Master File Tracking Dec 1, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Dec 1, 2023
Copy link

melvin-bot bot commented Dec 1, 2023

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Dec 1, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.6-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-12-08. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

  • @koko57 does not require payment (Contractor)

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Dec 8, 2023
Copy link

melvin-bot bot commented Dec 11, 2023

@koko57, @grgia Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Copy link

melvin-bot bot commented Dec 13, 2023

@koko57, @grgia Eep! 4 days overdue now. Issues have feelings too...

@grgia
Copy link
Contributor Author

grgia commented Dec 13, 2023

Closing as complete 🎊

@grgia grgia closed this as completed Dec 13, 2023
@melvin-bot melvin-bot bot removed the Overdue label Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2
Projects
No open projects
Status: Done - Live
Development

No branches or pull requests

4 participants