flutter icon indicating copy to clipboard operation
flutter copied to clipboard

Added new RefreshIndicator widget to be used with SliverAppBar.

Open elbeicktalat opened this issue 2 years ago • 9 comments

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.

elbeicktalat avatar Jun 10 '22 09:06 elbeicktalat

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.

Piinks avatar Jun 10 '22 19:06 Piinks

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!

elbeicktalat avatar Jun 10 '22 22:06 elbeicktalat

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.

Piinks avatar Jun 13 '22 22:06 Piinks

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

Piinks avatar Jun 13 '22 22:06 Piinks

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:

  1. The indicator can't begin behind of SliverAppBar.
  2. The CustomScrollView must have a physics of type BouncingScrollPhysics and it should be AlwaysScrollableScrollPhysics.

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!

elbeicktalat avatar Jun 14 '22 13:06 elbeicktalat

  • 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!

Piinks avatar Jun 21 '22 19:06 Piinks

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!

elbeicktalat avatar Jun 22 '22 17:06 elbeicktalat

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. :)

Piinks avatar Jun 22 '22 18:06 Piinks

@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.

Hixie avatar Sep 20 '22 23:09 Hixie

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!

elbeicktalat avatar Sep 24 '22 17:09 elbeicktalat

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!

Piinks avatar Dec 16 '22 00:12 Piinks