Skip to content

approval flow loading text#1419

Merged
bergarces merged 15 commits into
mainfrom
approval-flow-loading-text
Jun 19, 2023
Merged

approval flow loading text#1419
bergarces merged 15 commits into
mainfrom
approval-flow-loading-text

Conversation

@bergarces
Copy link
Copy Markdown
Contributor

@bergarces bergarces commented Jun 9, 2023

Description

Adds a method to set loading text for the approval flow pages.

Changes

  • ADDED: Adds method setFlowLoadingText to the controller

References

Fixes https://github.com/MetaMask/MetaMask-planning/issues/567

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

Comment thread packages/approval-controller/src/ApprovalController.ts Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Anyone is of the opinion that endFlow should set the loading text back to null? It might be inconvenient in the event of an approval flow within another approval flow, but I think that's going to be a very rare occurrence.

On the other hand, not clearing it after a previous approval flow could cause some interesting bugs depending in which order approval flows happen.

An alternative to this would be to add a loadingText parameter when a flow starts and set it back to default if nothing is passed.

And even though the ticket explicitly says not to, another alternative would be for the loading text to be linked to an approval flow.

Copy link
Copy Markdown
Member

@matthewwalsh0 matthewwalsh0 Jun 9, 2023

Choose a reason for hiding this comment

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

I initially wanted to do it per flow but the issue is ownership. If a nested flow exists, is the parent flow loading text displayed as it was the initial flow and has more high level context, or is the child flow loading text shown as it's more specific and more aware of what is happening?

On reflection, perhaps there is a compromise where the loading text is per flow and so setFlowLoadingText has a flowId argument, and by default the nested flow text is displayed as it's most relevant to the current work, unless the parent flow wants to be stubborn and was started with an option such as overrideNestedLoadingText?

We could be extra thorough and also have a ignoreNestedLoadingText which simply ignores all nested text even when there is no parent text.

Clearing the text on endFlow would then make sense, but not having default loading text to start with as we can't really know that until the first approval is requested.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So...

Without doing text by flowId, we can't just clear the text at endFlow, as that would inadvertently clear the parent loading text.

If we attach the flow to a specific flowId, then it won't matter. We can even come up with ways of inheriting the parent text if one is not provided.

Having said that, it's going to be difficult to predict what the functionality should exactly be if we don't have any use case for a nested approval flow.

@bergarces bergarces marked this pull request as ready for review June 9, 2023 15:06
@bergarces bergarces requested a review from a team as a code owner June 9, 2023 15:06
@bergarces bergarces marked this pull request as draft June 9, 2023 15:07
@bergarces bergarces marked this pull request as ready for review June 9, 2023 15:19
@bergarces bergarces merged commit e0a0de2 into main Jun 19, 2023
@bergarces bergarces deleted the approval-flow-loading-text branch June 19, 2023 10:42
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.

4 participants