craiglabenz commented 5 years ago

Package Proposal: Pull to Reveal

Dependency name (as used in pubspec.yaml): pull_to_reveal Current pub.dev Link: https://pub.dev/packages/pull_to_reveal Description: Wrapper around a ListView that reveals a given element when the list is pulled down - most likely for a search bar Current maintainer: Craig Labenz, craig.labenz@gmail.com, @craiglabenz Needs new maintainer after transfer: No New maintainer (if applicable): N/A Reason for transfer: Want to give back! Comments: :wave:

jeroen-meijer commented 5 years ago

Hi there,

Thanks for submitting your package. Seems quite useful 😁

I've reviewed the codebase and have found the following issues, sorted by importance.

Inconsistent snapping. When scrolling the list a certain way, the top search bar won't snap into place and is selectable even when not fully done animating yet. As far as I'm aware, this is not expected behaviour since most apps will snap the search bar back to either the closed or open state depending on if the user has pulled the search bar past the half-way point. Please either change your plugin's behaviour to match this, or add a parameter to the widget (something like bool snapIntoPlace = true) that dictates what should be done in these cases. (Check the included zip for reference.)
Inconsistent animation. When stopping and continuing scrolling the list, the underlying list will sometimes scroll a bit before pulling the search bar down. This makes for some weird scrolling behaviour. Please have a look and adjust the scrolling behaviour. (Check the included zip for reference.)
Documentation. While there seems to be some documentation (on the AlwaysBouncableScrollPhysics for example), the rest of the documentation is either missing, improperly formatted (using // instead of ///) or contains slightly incorrect grammar (such as missing periods). Please ensure all classes and fields have proper documentation. (Check the Dart doc guide for reference.)
Unintuitive example. The included example might not be the intuitive in my opinion. The search bar seems to filter on numbers lower that its input, instead of searching for a particular number. Although this is a minor issue, you might want to rework the example to better represent a real world use case, such as searching for a name in a list of names (like ["John", "Bob", "Alice"], where searching for "o" will change the shown list to ["John", "Bob"]).
No control over animation. The zoom animation is nice but I can imagine a lot of developers don't necessarily want this type of animation in their app. Please consider adding a animationType parameter that taken in a PullToRevealAnimationType enum instance (and include values such as .zoom and .none for the current zoom effect and for no animation, i.e. regular scrolling, respectively.)

Please make these changes and tell me when you've done so in this issue. If you have any questions, don't hesitate to ask. 👍🏻

Best regards.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 5 years ago

This issue has been automatically closed because it has not had any recent activity and was marked with "no response". Feel free to reply to get it reopened.

craiglabenz commented 5 years ago

I still want to respond to @jeroen-meijer's excellent feedback, but for now I'm in the middle of a cross country move. I hope to get to this soon!

jeroen-meijer commented 5 years ago

No problem! Good luck with the move 👍🏻

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 5 years ago

This issue has been automatically closed because it has not had any recent activity and was marked with "no response". Feel free to reply to get it reopened.

jeroen-meijer commented 5 years ago

@craiglabenz I need to get this bot fixed...

jeroen-meijer commented 5 years ago

Hi there @craiglabenz,

Hope moving went well! Any ETA on updates for this package?

craiglabenz commented 5 years ago

Thanks for your patience, @jeroen-meijer! I just hopped back into this code and addressed each of your suggested changes. Lots of good code improvements came out of those enum suggestions! :smile:

jeroen-meijer commented 5 years ago

Cool! Let me know when your changes are committed and you want the package to be re-evaluated.

craiglabenz commented 5 years ago

Oops, I guess I failed to make this at all obvious, but I have already bumped the version in pub! Here's the commit :smile:

jeroen-meijer commented 5 years ago

Awesome. I’ll have a look sometime soon. Thanks 👍🏻

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

craiglabenz commented 5 years ago

Aggressive bot is aggressive!

jeroen-meijer commented 5 years ago

Sorry about that! I really oughta disable it until I find a way to make it do its damn job properly 😅

stale[bot] commented 4 years ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

jeroen-meijer commented 4 years ago

I thought I disabled that bot...

jeroen-meijer commented 4 years ago

Hi there,

Thanks for waiting, I know this package has been open for approval for a long time.

I've taken a look again, and first off, great work on the changes!

To do before approval

As soon as these issues are fixed, the package will get rereviewed and added to the Flutter Community.

Documentation. Running flutter analyze with API docs linter options shows 37 public members lack documentation. To see which parts are missing, try adding analysis_options.yaml file to the root of the project (next to lib, pubspec.yaml, etc.) and put this in it:
```
linter:
rules:
- public_member_api_docs
- package_api_docs
- slash_for_doc_comments
```

Changes for the future

These are changes that would be nice to have in the future, either by you or some other community member.

Scroll behaviour. It seems that the scroll behaviour is not the same as it is on iOS. If you want to search something in a list in iOS, you have to scroll all the way to the top of the list and then pull to reveal the search bar. If this scroll behaviour is intentional, then no problem, but it would be nice to show this to users through a gif or animation.
Animation is hardcoded? It seems the animation for the revealable is hardcoded, or otherwise not buildable by the user. It might be a nice feature to be able to build your own animation based on the offset. For inspiration, check out CupertinoSliverRefreshControl's RefreshControlIndicatorBuilder.

Overall, great work. Nice tests, too. 👍🏻 As soon as the docs are fixed, we'll get this baby in the community.

Best regards 👋🏻

jeroen-meijer commented 4 years ago

Hi there 👋🏻

Are there updates on this?

jeroen-meijer commented 4 years ago

Hi there,

I'm closing this due to inactivity. Feel free to reopen it whenever you're ready to continue to proposal process.

Thanks! 👋🏻

fluttercommunity / community