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

Transfer Funds in progress dialog state management #1234

Merged

Conversation

erdemyerebasmaz
Copy link
Contributor

This PR addresses #1232

Instead of popping the last item on navigation stack, the route(dialog) itself is removed if it's active.

@erdemyerebasmaz erdemyerebasmaz added the bug Something isn't working label Oct 16, 2023
if (state.transferringOnChainDeposit != true) {
_pop();
if (state.transferringOnChainDeposit != true && _currentRoute.isActive) {
Navigator.of(context).removeRoute(_currentRoute);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you can access build context in initState.

Copy link
Contributor Author

@erdemyerebasmaz erdemyerebasmaz Oct 16, 2023

Choose a reason for hiding this comment

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

These are not accessed in initState per se but after subscription to a stream has established, we're also checking if _currentRoute.isActive beforehand.

It's not really needed on this example but what we can do to make things more robust is scheduling the stream subscription logic once the context is available as such:

    WidgetsBinding.instance.addPostFrameCallback((_) {
      /// Stream Subscription Logic
    });

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we wish to do this in the initstate instead of calling this in didChangeDependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to subscribe to the stream only once and didChangeDependencies may be called multiple times per widget lifecycle. We'd need some preventative measures in didChangeDependencies, we do have some examples using _isInit variable on breezmobile.

Copy link
Contributor

@ubbabeck ubbabeck Oct 16, 2023

Choose a reason for hiding this comment

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

Got it, thanks for clarifying.

Copy link
Contributor

@JssDWt JssDWt left a comment

Choose a reason for hiding this comment

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

I don't really understand. But I have no objections. LGTM!

@roeierez
Copy link
Member

Can we merge that?

Copy link
Contributor

@ubbabeck ubbabeck left a comment

Choose a reason for hiding this comment

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

LGMT

@erdemyerebasmaz erdemyerebasmaz merged commit a850fb1 into master Oct 17, 2023
1 check passed
@erdemyerebasmaz erdemyerebasmaz deleted the transfer_funds_in_progress_dialog_state_mgmt branch December 4, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants