Remove WillPopScope

[remonke]

Fix #430

[joanpablo]

Thanks @remonke

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.57%. Comparing base (94c928d) to head (42e20b4).

❗ Current head 42e20b4 differs from pull request most recent head 0568710. Consider uploading reports for the commit 0568710 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #431      +/-   ##
==========================================
- Coverage   95.58%   95.57%   -0.01%     
==========================================
  Files          66       66              
  Lines        1359     1356       -3     
==========================================
- Hits         1299     1296       -3     
  Misses         60       60              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[vasilich6107]

@remonke could you clarify why you removed the it completely instead of migration to PopInvokedCallback?

[remonke]

@remonke could you clarify why you removed the it completely instead of migration to PopInvokedCallback?

ReactiveFormBuilder manages the form state interally, and such there is no way to compute the value based on the form state (such as preventing pop if the form is disabled). If anything, I suggest making a separate widget that implements both canPop and onPopInvoked as functions with the formGroup as an argument, so the value would be computed based on the current form state.

Here's an example of how this could be implemented:

class ReactiveFormPopScope extends StatelessWidget {
  final bool Function(FormGroup formGroup)? canPop;
  final bool Function(bool didPop, FormGroup formGroup)? onPopInvoked;
  final Widget child;

  const ReactiveFormPopScope({
    super.key,
    this.canPop,
    this.onPopInvoked,
    required this.child,
  });

  @override
  Widget build(BuildContext context) {
    return ReactiveFormConsumer(
      builder: (context, formGroup, _) {
        return PopScope(
          canPop: canPop != null ? canPop!(formGroup) : true,
          onPopInvoked: onPopInvoked != null
              ? (didPop) => onPopInvoked!(didPop, formGroup)
              : null,
          child: child,
        );
      },
    );
  }
}

[remonke]

@joanpablo
@vasilich6107

I decided to implement canPop and onPopInvoked to take callbacks, so they take the formGroup as an argument (just like in onSubmitted on ReactiveTextField. This way the value can be easily computed based on the state of the form (such as preventing pop when a form is disabled, as I use it pretty commonly for loading state). However, this is a breaking change so I suggest releasing a new major version. Let me know if there is anything I could improve (documentation potentially?), and if not, can you please merge it?

[joanpablo]

Thanks for the contribution @remonke.

Why create a new widget instead of using directly the PopScope here?

[remonke]

This is the way they do it in the Flutter Form widget, just decided to copy this.

[remonke]

It would also improve performance I guess, since each form change would cause the widget to get rebuild.

[joanpablo]

Hi @remonke

I like the idea of this PR but before I can approve it there are some issues I believe should be addressed first:

1- Some tests are failing with this PR. The tests should be adapted and new tests should be implemented if necessary.
2- Some code formatting issues are present in this PR.
3- Would you mind updating the pubspec.yaml with the minimum required flutter version for this PR?
4- Would you mind updating the CHANGELOG.md and adding version 17.0.0 since this is going to be a Breaking Changes release?

Thanks a lot for the great work.

[remonke]

I updated the CHANGELOG.md and pubspec.yaml, but I tried running dart analyze, and it found no issues, so I don't know what's going on.

[joanpablo]

Hi @remonke,

Oh I know what is happening we need to update the Flutter version used in the build CI

[joanpablo]

Right now @remonke the CI is using the Flutter 3.10.0 to build the project, but we need to update it to the minimum Flutter version that introduces the PopScope

[joanpablo]

BTW in this PR you should update the pubspec.yaml with that minimum version that introduces the PopScope widget

[remonke]

Sorry, I'm not familiar with CI/CD. Any reason you can't fix it yourself?