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

Reason for using child/children instead of sliver/slivers #14

Closed
jamesblasco opened this issue Oct 7, 2020 · 5 comments
Closed

Reason for using child/children instead of sliver/slivers #14

jamesblasco opened this issue Oct 7, 2020 · 5 comments
Labels
question Further information is requested wontfix This will not be worked on

Comments

@jamesblasco
Copy link

jamesblasco commented Oct 7, 2020

First, thanks for this amazing package.

Slivers are a very powerful set of Widgets and this package makes it even more incredible.

I would like to suggest to rename the params children and child in the sliver widgets of this library to slivers and sliver.

Inside the Flutter framework, the widgets that only accept slivers use that name convention to indicate a normal widget should not be used. eg SliverSafeArea, SliverPadding, CustomScrollView

Calling it child and children is confusing and prompt to use normal Widgets and get runtime errors.

Happy to help with a PR.

@Kavantix
Copy link
Owner

Kavantix commented Oct 7, 2020

Thanks for the suggestion.
However, naming them child/children was a conscious decision (I even had them as sliver/slivers at first).
The tooling used for wrapping with and removing widgets can only deal with child/children so having sliver/slivers breaks these function which can be quite annoying when developing in flutter.
Also, my SliverStack widget actually accepts both slivers and box children so to make it consistent within my package I use these terms on all of them.
And maybe more importantly, it doesn't actually prevent any runtime errors by having to use the sliver name for the parameter, especially since someone using my package will probably be using custom widgets that return slivers (so not even the names of the widgets will help in that case)

Moreover, I actually am thinking of supporting both box and sliver children in all of my widgets, there is no technical reason why only RenderSliver children should be allowed as defaulting to the SliverToBoxAdapter logic is trivial, this should also fix the issue that whenever you get an error in a sliver widget the whole page breaks because the error is not a RenderSliver.
And clean up all SliverToBoxAdapters

So TL;DR I don't plan on changing this as I disagree with this naming by the Flutter framework and want consistency in my own package

I'll leave this issue open for future reference and possibility for discussion

@jamesblasco
Copy link
Author

Nice to hear your point of view 🙂

I also hope in the future SliverToBoxAdapter is not needed and slivers become more flexible to use other widgets.

Leaving my personal experience here, for all custom widgets that contain a RenderSliver I add the prefix Sliver in the class name. The params that need to be a widget that contains a RendersSliver I called them Widget sliver.

I completely understand why you don't and why you prefer to keep it that way on your projects.

Again, thank you for the package

@jamesblasco jamesblasco changed the title Param children should be called slivers Rename children params to slivers for widgets that need to have a RenderSliver Oct 7, 2020
@Kavantix Kavantix added question Further information is requested wontfix This will not be worked on labels Jan 6, 2021
@Kavantix Kavantix pinned this issue Jan 23, 2021
@Kavantix
Copy link
Owner

I have pinned this such that it can be closed but will stay easy to find

@Kavantix Kavantix changed the title Rename children params to slivers for widgets that need to have a RenderSliver Reason for using child/children instead of sliver/slivers Jan 23, 2021
@Peng-Qian
Copy link

Personally, I think from an API perspective, sliver/slivers will keep consistency to the Flutter Sliver part, I was confused when I first time see them (I thought it should be normal widgets instead of sliver widget).

But if child/children make the project easy to maintain, I think it is a fair trade-off.

An awesome project really!

@Kavantix
Copy link
Owner

In #26 (released as 0.2.1-rc) I just added RenderBox support to MultiSliver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants