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

Remove unnecessary mounted state from Home component #13202

Merged
merged 1 commit into from
Jan 4, 2022

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jan 4, 2022

The mounted state was used to derive state from props before the first render of the Home component. Instead this state is now derived in the constructor, which is also run before the first render. This should behave exactly the same, except now we don't need the mounted state or the deriveStateFromProps function anymore.

The call to closeCurrentWindow that was made in componentDidUpdate has been moved to the constructor as well. There is no need to delay that call, and this saves us from having to compare current with previous state in that lifecycle function.

Manual testing steps:

  • All confirmation flows rely upon this logic. I'd suggest testing every confirmation you can think of, and ensuring that the Home screen is rendered correctly in the fullscreen UI when it's open at the same time as the confirmation.

The `mounted` state was used to derive state from props before the
first render of the Home component. Instead this state is now derived
in the constructor, which is also run before the first render. This
should behave exactly the same, except now we don't need the `mounted`
state or the `deriveStateFromProps` function anymore.

The call to `closeCurrentWindow` that was made in `componentDidUpdate`
has been moved to the constructor as well. There is no need to delay
that call, and this saves us from having to compare current with
previous state in that lifecycle function.
@Gudahtt Gudahtt marked this pull request as ready for review January 4, 2022 15:21
@Gudahtt Gudahtt requested a review from a team as a code owner January 4, 2022 15:21
@Gudahtt Gudahtt requested a review from ryanml January 4, 2022 15:21
@metamaskbot
Copy link
Collaborator

Builds ready [43844a4]
Page Load Metrics (1293 ± 78 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint781397692525252
domContentLoaded10641693129316278
load10641693129316278
domInteractive10641693129316278

@darkwing
Copy link
Contributor

darkwing commented Jan 4, 2022

All confirmations and suggestions worked for me. Well done!

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

The confirmations work on my device. LGTM!

@Gudahtt Gudahtt merged commit 75b9f71 into develop Jan 4, 2022
@Gudahtt Gudahtt deleted the simplify-home-routing-logic branch January 4, 2022 22:49
@github-actions github-actions bot locked and limited conversation to collaborators Jan 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants