aagarwal1012 / Liquid-Pull-To-Refresh

🔁 A custom refresh indicator for flutter.
https://pub.dev/packages/liquid_pull_to_refresh
MIT License
1.24k stars 92 forks source link

Anim speed factor and refactoring code #47

Closed AadumKhor closed 4 years ago

AadumKhor commented 4 years ago

This commit contains the changes requested in the comments of commit fdf49fd and also changes suggested in #33 (only first point as of now). Updating readme accordingly. Please review @aagarwal1012.

codecov[bot] commented 4 years ago

Codecov Report

Merging #47 into master will increase coverage by 0.62%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
+ Coverage   86.51%   87.13%   +0.62%     
==========================================
  Files           3        3              
  Lines         430      443      +13     
==========================================
+ Hits          372      386      +14     
+ Misses         58       57       -1     
Impacted Files Coverage Δ
lib/liquid_pull_to_refresh.dart 89.29% <100.00%> (+0.69%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 33f72c7...3d561a7. Read the comment docs.

aagarwal1012 commented 4 years ago

@AadumKhor, please update this branch. Also, Travis CI failed, check via https://travis-ci.com/github/aagarwal1012/Liquid-Pull-To-Refresh/jobs/326418327#L286.

AadumKhor commented 4 years ago

@AadumKhor, please update this branch. Also, Travis CI failed, check via https://travis-ci.com/github/aagarwal1012/Liquid-Pull-To-Refresh/jobs/326418327#L286.

It says named parameter 'scrollController' is not defined in main.dart file. There is no named parameter with that name on this branch. I have removed it already. Is this referring to the main.dart file on master branch?

aagarwal1012 commented 4 years ago

@AadumKhor, I have approved this PR, you can now do a squash merge. After that, if you are done with fixing issues, I will do a version update.

AadumKhor commented 4 years ago

@aagarwal1012 it will take some time to fully resolve #33 so I would suggest adding current features in the release while I continue working on other issues.

Also #33 suggests having an option of replacing the custom progress indicator with the built in one. I honestly think otherwise and would like your opinion on it. I am working on the second point though and will come up with a fix.

Making a squash merge with the current state of code.

AadumKhor commented 4 years ago

@aagarwal1012 please update example/lib/main.dart file. It has scrollController as a parameter which is incorrect as of last merge.

aagarwal1012 commented 4 years ago

@AadumKhor, I have updated the example app. For the spinning indicator part, we can surely replace it with CircularProgressIndicator as it will give better customization. Also, I will suggest we can do all the fixes and then we can do a v2.0.0 release.

AadumKhor commented 4 years ago

@aagarwal1012 Sure. I will add the indicator first and then solve the "manually stop refresh" part.

AadumKhor commented 4 years ago

@aagarwal1012 The CircularProgressIndicator looks something like this. Right now it is just a boolean to show whether you want to see the built in ProgressIndicator or not. Do we also want to make it completely customizable or is it OK to keep it as a second option that we cannot change?

It looks something like this right now. circular_progress_gif

Looking forward to some feedback.

aagarwal1012 commented 4 years ago

@AadumKhor, I think we should not change it as there may be some chances which can lead to loss of the essence of the package. Also, the refresh indicator mentioned in the above image doesn't look good (borders are visible).

AadumKhor commented 4 years ago

@aagarwal1012 That is exactly what I thought which is why I asked. And the second point of #33 is also counter-intuitive. If you have initiated the refresh then that process should not stop manually. It is not available in the built in RefreshIndicator as well. I guess we should close that issue since those additions are not necessary.

aagarwal1012 commented 4 years ago

@AadumKhor, I completely agree with you, mention this thing in the issue and close it.