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.

Sign up for GitHub

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

Jump to bottom

Refactor WorkspaceInitialPage to no longer use withFullPolicy #10444

Merged
merged 3 commits into from
Aug 22, 2022
Merged

Refactor WorkspaceInitialPage to no longer use withFullPolicy #10444

merged 3 commits into from
Aug 22, 2022

Conversation

yuwenmemon
Copy link
Contributor

@yuwenmemon yuwenmemon commented Aug 18, 2022 •
edited
Loading

@arosiclair Please review

Details

Refactor WorkspaceInitialPage to no longer use withFullPolicy and use the FullPageNotFoundView instead.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/218369

Tests/QA

  • Using the JS console, switch the network tab to off
  • Navigate to the workspace, make sure you are still able to and you load the page fully like before.
  • For all of the above make sure there are no JS console errors while you do any of this.

@yuwenmemon yuwenmemon requested a review from arosiclair August 18, 2022 07:44
@yuwenmemon yuwenmemon self-assigned this Aug 18, 2022
@yuwenmemon yuwenmemon requested a review from a team as a code owner August 18, 2022 07:44
@melvin-bot melvin-bot bot requested review from TomatoToaster and removed request for a team August 18, 2022 07:44
arosiclair
arosiclair requested changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@arosiclair arosiclair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from withFullPolicy removal, everything else looks good!

src/pages/workspace/WorkspaceInitialPage.js Outdated
@@ -240,10 +247,12 @@ WorkspaceInitialPage.displayName = 'WorkspaceInitialPage';

export default compose(
withLocalize,
withFullPolicy,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion, but we're no longer looking to remove withFullPolicy completely. Instead, we'll just be updating it to not fetch policy data so you can keep it here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... Yes, I am confused 😅 . Do we need withFullPolicy here - why keep it? Also, the withFullPolicy code is still loading the full policy - when are we removing that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So withFullPolicy also tracks the last visited workspace/policy so we shouldn't remove that functionality.

We'll remove the policy loading with this GH but this is only safe to do once all the other refactors are done (if we do it before, these pages will have no data).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay great - makes sense thanks for clarifying!

@yuwenmemon
Copy link
Contributor Author

Updated and retested!

@arosiclair arosiclair merged commit c3c6877 into main Aug 22, 2022
@arosiclair arosiclair deleted the yuwen-workspaceInitialPage branch August 22, 2022 14:18
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify OSBotify mentioned this pull request Aug 23, 2022
Closed
48 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @arosiclair in version: 1.1.89-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arosiclair arosiclair arosiclair approved these changes

@TomatoToaster TomatoToaster Awaiting requested review from TomatoToaster

Assignees

@yuwenmemon yuwenmemon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuwenmemon @OSBotify @arosiclair