Skip to content

Tighten up loading indication logic#10103

Merged
rekmarks merged 8 commits intodevelopfrom
tighten-up-loading-indication-logic
Dec 18, 2020
Merged

Tighten up loading indication logic#10103
rekmarks merged 8 commits intodevelopfrom
tighten-up-loading-indication-logic

Conversation

@rekmarks
Copy link
Copy Markdown
Member

@rekmarks rekmarks commented Dec 17, 2020 •
edited
Loading

Ensures that hideLoadingIndication is always called in all actions that call showLoadingIndication. It's unclear how many of these actions were failing to hide the loading indication, because other actions superset hideLoadingIndication.

At the very least, updateTransaction was probably failing to hide the loading indication in the error case.

This PR also refactors a lot of actions to call hideLoadingIndication once in finally blocks as opposed to multiple times across try and catch blocks. We avoided making changes to functions using Promise methods, because Promise.finally is not supported by Waterfox, and it's not properly transpiled by Babel.

@rekmarks rekmarks requested a review from a team as a code owner December 17, 2020 21:56
@rekmarks rekmarks requested a review from darkwing December 17, 2020 21:56
@rekmarks rekmarks mentioned this pull request Dec 17, 2020
Closed
@darkwing
Copy link
Copy Markdown
Contributor

darkwing commented Dec 17, 2020

Trying to figure out the e2e failure, but I'm also seeing this:

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
    in ConfirmTransactionBase (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in withRouter(Connect(ConfirmTransactionBase)) (created by ConfirmSendEther)
    in ConfirmSendEther (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in withRouter(Connect(ConfirmSendEther)) (created by Context.Consumer)
    in Route (created by ConfirmTransaction)
    in Switch (created by ConfirmTransaction)

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Dec 17, 2020

Builds ready [bccba6b]
Page Load Metrics (548 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint39715384
domContentLoaded3446415469947
load3466425489947
domInteractive3446415469947

darkwing
darkwing previously approved these changes Dec 17, 2020
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Dec 18, 2020

Builds ready [c26e0be]
Page Load Metrics (516 ± 48 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31584763
domContentLoaded31973251510048
load32073351610048
domInteractive31873251410048

Gudahtt
Gudahtt reviewed Dec 18, 2020
View reviewed changes
Gudahtt
Gudahtt approved these changes Dec 18, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@rekmarks rekmarks merged commit 4b766fa into develop Dec 18, 2020
@rekmarks rekmarks deleted the tighten-up-loading-indication-logic branch December 18, 2020 05:34
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

+1 more reviewer

@darkwing darkwing darkwing left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rekmarks @darkwing @metamaskbot @Gudahtt