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

Add ability to control RefreshProgressIndicator's distance from edges. #30208

Closed

Conversation

noordawod
Copy link
Contributor

@noordawod noordawod commented Mar 29, 2019

Description

This PR allows the user to set the distance of RefreshProgressIndicator from the screen edges.

Current behavior: the embedded RefreshProgressIndicator used internally by RefreshIndicator is always placed at the very top/bottom of the screen.

New behavior: the user is able to set an edgeDistance property which controls how far the RefreshProgressIndicator is from the top/bottom edges of the screen. To be backward-compatible, the default value of edgeDistance is 0.

Use case

Let's have a ListView and a top-positioned AppBar as children in a Stack. Naturally, you need to give the ListView some padding on top in order for the top items not to be covered by the AppBar. So that means that when pulling to refresh, the RefreshProgressIndicator should start to appear where the app bar ends, and not from the top of the screen.

Related Issues

No issues involved.

Tests

I have not added any tests!

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@dnfield dnfield added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Mar 29, 2019
@xster
Copy link
Member

xster commented Apr 6, 2019

cc @willlarche is this material spec compatible?

@willlarche
Copy link
Member

Sounds great!

@noordawod
Copy link
Contributor Author

@willlarche @xster any update on this? it's relatively a straightforward, backward-compatible change.

@willlarche
Copy link
Member

It's not something on our radar at the moment. If you'd like to work up a PR we'd be happy to help you land it!

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

This needs a test - if you need help adding one, please feel free to ask!

Copy link
Contributor Author

@noordawod noordawod left a comment

Choose a reason for hiding this comment

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

Added Dartdoc to explain a bit more about the variable.

@noordawod
Copy link
Contributor Author

This needs a test - if you need help adding one, please feel free to ask!

Yup, no experience with testing unfortunately.. I'd love help with testing!

@@ -96,6 +96,7 @@ class RefreshIndicator extends StatefulWidget {
Key key,
@required this.child,
this.displacement = 40.0,
this.edgeDistance = 0.0,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should assert this is not null, and document it as such.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

Sorry I lost track of this!

If you take a look at the tests in refresh_indicator_test.dart in this repo, you should be able to just copy the one like this:

testWidgets('RefreshIndicator - top - position', (WidgetTester tester) async {
    refreshCalled = false;
    await tester.pumpWidget(
      MaterialApp(
        home: RefreshIndicator(
          onRefresh: holdRefresh,
          child: ListView(
            physics: const AlwaysScrollableScrollPhysics(),
            children: const <Widget>[
              SizedBox(
                height: 200.0,
                child: Text('X'),
              ),
            ],
          ),
        ),
      ),
    );

    await tester.fling(find.text('X'), const Offset(0.0, 300.0), 1000.0);
    await tester.pump();
    await tester.pump(const Duration(seconds: 1));
    await tester.pump(const Duration(seconds: 1));
    expect(tester.getCenter(find.byType(RefreshProgressIndicator)).dy, lessThan(300.0));
  });

And update it so that you set the new edgeDistance parameter and assert that it's where we'd expect it to be positionally. Let me know if that's unclear!

Copy link

@TinchoOlivari TinchoOlivari left a comment

Choose a reason for hiding this comment

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

I tested these changes and they work perfectly.
Personally I think it's something really necessary. I was looking for how to make this modification and I had trouble finding it. This is the best way to implement it.

Flutter version: flutter_windows_v1.9.1+hotfix.2-stable\flutter
Android version: 8.1.0

@dnfield
Copy link
Contributor

dnfield commented Dec 29, 2019

@noordawod will you be able to add that test?

@noordawod
Copy link
Contributor Author

Yes, I will add it next year just after the holidays, @dnfield!

@dnfield
Copy link
Contributor

dnfield commented Feb 5, 2020

Without tests, we can't land this PR. I'm going to close this to get it off of the review queues. If you decide to add tests, please ping it and we can reopen - or someone is welcome to take this over in a new PR with tests.

@dnfield dnfield closed this Feb 5, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants