flutter
flutter copied to clipboard
Added new RefreshIndicator widget to be used with SliverAppBar.
This PR. introduce a new widget called SliverRefreshIndicator
which is the material equivalent to CupertinoSliverRefreshControl
, both widget are subclasses of BaseSliverRefreshControl
which holds the shared logic.
It is also provide a new named constructor partiallyRevealed
to the existing RefreshProgressIndicator
, which allows the getting RefreshProgressIndicator
based on passed progress value.
Fix #54272
Pre-launch Checklist
- [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
- [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
- [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
- [X] I signed the [CLA].
- [X] I listed at least one issue that this PR fixes in the description above.
- [X] I updated/added relevant documentation (doc comments with
///
). - [X] I added new tests to check the change I am making, or this PR is [test-exempt].
- [ ] All existing and new tests are passing.
Self-pinging to put this in my inbox for review. Thank you for putting this together. It looks like the failing tests are either broken doc references, analyzer issues or an issue with the sample code you are adding.
It sounds like this PR is really two features as described above. Can this be split into two separate PRs then? It makes it easier to review, but more importantly, if 1 of 2 features causes an issue, it will be reverted, meaning both would have to be reverted rather than just the one.
I think it doesn't make sense splitting this in two PR. since they are related to each other.
In the end, if you see that essential splitting this in two PR. then I'll do it.
Thank you!
I think it doesn't make sense splitting this in two PR. since they are related to each other.
One way to tell is, how does adding partiallyRevealed
resolve https://github.com/flutter/flutter/issues/54272? If it does not resolve the issue, it should probably be a separate PR.
Also of note, the material version here does not currently match the native behavior.
This is the current behavior of RefreshIndicator:
https://user-images.githubusercontent.com/16964204/173460137-c534b7b1-31af-4370-88a6-7f326898ab09.mov
And this is the proposed sliver refresh here:
https://user-images.githubusercontent.com/16964204/173460196-be31f971-4b34-49da-85e1-a6e5bfe23019.mov
The behavior does not look quite right. Can you take a look?
This page should detail of the expected behaviors of this widget: https://material.io/design/platform-guidance/android-swipe-to-refresh.html#usage
Hi @Piinks,
I have fixed all errors, now the big deal is the behavior of the SliverRefreshIndicator
, since this widget becomes part of the slivers, then I hit the following:
- The indicator can't begin behind of
SliverAppBar
. - The
CustomScrollView
must have a physics of typeBouncingScrollPhysics
and it should beAlwaysScrollableScrollPhysics
.
With the actual structure can arrive to something like:
https://user-images.githubusercontent.com/59793332/173591445-81199c1c-84f4-4663-ace6-b063fcf86a58.mp4
If this doesn't seems to be right, then probably I'll reset The CupertinoSliverRefreshControl
and provide a dedicated implementation for SliverRefreshIndicatro
.
Thank you!
- The indicator can't begin behind of SliverAppBar.
- The CustomScrollView must have a physics of type BouncingScrollPhysics and it should be AlwaysScrollableScrollPhysics.
This breaks the design of the Material indicator, so I do not think we will want to make this change. If there is a way to change this proposal and match the design spec, we can definitely take another look!
For sure I can get out of this issue, but I have to revert a few changes like: having a Base implementation for both iOS and Material.
Should I allow RefreshIndicator
to warp the CustomListView,
or make a new refresh indicator widget which becomes part of CustomListView
?
Thanks!
Should I allow RefreshIndicator to warp the CustomListView, or make a new refresh indicator widget which becomes part of CustomListView?
Do you mean CustomScrollView? I am not sure! I would return to the issue you are trying to solve, and see what you think would be the best approach. I do have a proposal tat is almost finished that may resolve this, I would love your feedback on it when it is ready. :)
@elbeicktalat Will you be able to continue working on this? It's totally fine if not; we can take it over (though it might take longer). I'm just asking because I'm trying to clean out our review queue.
Hi @Hixie I'm really sorry for the delay in responding, I was working on this until @Piinks side to me that she has a solution for this, any way unfortunately I'm no more able to work on this, but I still able to give info if there is need.
Thanks, and again I'm sorry!
Hey @elbeicktalat, I am going to close this PR. I do still have plans to support this without breaking the current spec (https://github.com/flutter/flutter/pull/107182 was an earlier proposal), I just haven't circled back around to it yet. I really appreciate your contribution and feedback on what we need to be supporting better here. Thank you!