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

Revert "Revert "Hide header on wallet onboarding"" #18952

Conversation

nathanmsmith
Copy link
Contributor

@nathanmsmith nathanmsmith commented Aug 15, 2019

Reverts #18948, which reverted #18896. Also introduces a fix for the issue that caused the first revert to happen.

The issue was when a navigation action would occur on the wallets page, a user would be redirected to the onboarding page, no matter if they accepted the disclaimer or not. This was because the OnboardingOrWallets component that handles routing to either the onboarding or wallets screen would be default show the onboarding screen and route to the wallets page when:

  1. OnboardingOrWallets mounted and this.props.acceptedDisclaimer === true
  2. OnboardingOrWallets was updated and prevProps.acceptedDisclaimer === false && this.props.acceptedDisclaimer === true. The reason why we didn't just check if this.props.acceptedDisclaimer === true was to avoid unnecessary navigation and re-renders.

The issue came from this logic in 2. When a user who had accepted the disclaimer first navigated to the wallet page, the page would look fine, but any subsequent navigation (e.g., to another account or the account settings) through the subnavigatior on the page would trigger a re-render of OnboardingOrWallets, which would check the prevProps.acceptedDisclaimer === false && this.props.acceptedDisclaimer === true condition. In this case, the acceptedDisclaimer would be true both for the current and previous render and thus the navigation to the wallets page would not happen and the navigator would display the initial page, the onboarding page.

My solution to this is to swap the initial page from the onboarding page to the wallets page. When OnboardingOrWallets mounts and this.props.acceptedDisclaimer === false, it will navigate to the onboarding page. The onboarding page does not contain a subnavigator, so it won't trigger any navigation events that will cause a re-render of OnboardingOrWallets. The logic in 2 will remain; when a user accepts the disclaimer from the onboarding page, they will be navigated to the wallets page.

I also took this as an opportunity to remove some logic from WalletsAndDetails, a component rendered on the wallets page on desktop. It previously displayed the onboarding page before I implemented my navigation solution in #18896. Since it's only displayed when the disclaimer is accepted, it's redundant to have WalletsAndDetails check if the disclaimer is accepted and display the onboarding page if not.

@nathanmsmith nathanmsmith marked this pull request as ready for review August 15, 2019 15:53
@nathanmsmith nathanmsmith merged commit 4acaff5 into master Aug 15, 2019
@nathanmsmith nathanmsmith deleted the revert-18948-revert-18896-nathan/PICNIC-331-unopened-wallet-tab branch August 15, 2019 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants