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

Combined BlocBuilder/Listener #545

Closed
fl0cke opened this issue Sep 30, 2019 · 16 comments
Closed

Combined BlocBuilder/Listener #545

fl0cke opened this issue Sep 30, 2019 · 16 comments
Assignees
Labels
discussion Open discussion for a specific topic enhancement New feature or request feedback wanted Looking for feedback from the community
Projects
Milestone

Comments

@fl0cke
Copy link

fl0cke commented Sep 30, 2019

Is your feature request related to a problem? Please describe.
I often find myself in the situation where I need both bloc builders and bloc listeners in the same widget tree, e.g. for showing a loading indicator (builder) and displaying a snackbar on success (listener).

Describe the solution you'd like
A BlocBuilderListener Widget that accepts both a builder function and a listener would greatly reduce the clutter in my code and also reduce the overhead of having two stateful widgets that basically do the same. Alternatively, you could add a listener parameter to the existing BlocBuilder widget.

@felangel
Copy link
Owner

felangel commented Oct 1, 2019

Hi @fl0cke 👋
Thanks for opening an issue!

Can you please elaborate a bit on why you say this would reduce the clutter in your code?
If I'm understanding correctly, you're proposing to go from:

BlocListener<MyBloc, MyState>(
  listener: (context, state) { ... },
  child: BlocBuilder<MyBloc, MyState>(
    builder: (context, state) { ... }
  ),
),

to

BlocBuilderListener<MyBloc, MyState>(
  listener: (context, state) { ... },
  builder: (context, state) { ... },
),

I personally don't think it's cluttered as is and I'd be more concerned about confusing people because it isn't immediately obvious what BlocBuilderListener would do.

Thoughts?

@felangel felangel self-assigned this Oct 1, 2019
@felangel felangel added discussion Open discussion for a specific topic enhancement candidate Candidate for enhancement but additional research is needed feedback wanted Looking for feedback from the community question Further information is requested waiting for response Waiting for follow up labels Oct 1, 2019
@fl0cke
Copy link
Author

fl0cke commented Oct 1, 2019

Hey @felangel,
Thanks for the quick reply!

Yes, that's exactly what the proposed BlocBuilderListener would look like. Alternatively, we could add an optional listener parameter to the BlocBuilder to avoid introducing a new widget.

I agree that it's not that much clutter, but when i look at my usage of the two BlocX widgets, i often find myself using either just a BlocBuilder or a BlocBuilder nested in a BlocListener, but almost never a BlocListener on its own. So this would be good opportunity to reduce the number of widgets that are required to achieve a reactive UI + side effects.

Example: A common use case is submitting some sort of user input and displaying a loading indicator while doing so. On success, I want to both show a SnackBar (using the listener) and hide the loading indicator (i.e. rebuild the UI using the builder).

However, I'm also just getting started with BLOC so you probably know better what's good for the library 👍

@felangel felangel removed the waiting for response Waiting for follow up label Oct 3, 2019
@felangel felangel added this to In progress in bloc Oct 10, 2019
@bigworld12
Copy link

bigworld12 commented Oct 10, 2019

I would suggest the merge to have the same signature as ListView and ListView.builder

e.g.

//normal BlocListener, listener is triggered on state change, child doesn't get rebuilt on state change
BlocView<MyBloc,MyState>(
  listener: (context, state) { ... }, 
  child: Container()
)

//works as BLocBuilder and BlocListener combined, listener and builder get triggered on state change
BlocView<MyBloc,MyState>.builder(
  listener: (context, state) { ... }, //listener here is optional
  builder: (context, state) { ... },
)

(this also goes well with my proposal here #560)
e.g.

BlocView<MyBloc>(
  listener: (context, state) { ... }, 
  child: Container()
)

BlocView<MyBloc>.builder(
  listener: (context, state) { ... }, //listener here is optional
  builder: (context, state) { ... },
)

now we can also take this to the next level by listening to multiple Blocs

BlocView<BlocA,BlocB,BlocC>(
  listener: (context, stateA,stateB,stateC) { ... }, 
  child: Container()
)

BlocView<BlocA,BlocB,BlocC>.builder(
  listener: (context, stateA,stateB,stateC) { ... }, //listener here is optional
  builder: (context, stateA,stateB,stateC) { ... },
)

@felangel felangel moved this from In progress to To do in bloc Oct 28, 2019
@cgaaf
Copy link

cgaaf commented Oct 29, 2019

I agree that

Hey @felangel,
Thanks for the quick reply!

Yes, that's exactly what the proposed BlocBuilderListener would look like. Alternatively, we could add an optional listener parameter to the BlocBuilder to avoid introducing a new widget.

I agree that it's not that much clutter, but when i look at my usage of the two BlocX widgets, i often find myself using either just a BlocBuilder or a BlocBuilder nested in a BlocListener, but almost never a BlocListener on its own. So this would be good opportunity to reduce the number of widgets that are required to achieve a reactive UI + side effects.

Example: A common use case is submitting some sort of user input and displaying a loading indicator while doing so. On success, I want to both show a SnackBar (using the listener) and hide the loading indicator (i.e. rebuild the UI using the builder).

However, I'm also just getting started with BLOC so you probably know better what's good for the library 👍

I agree that reducing unnecessary nesting would be preferable. I do like the idea of adding an optional listener parameter to the bloc builder. I agree with you @felangel that building a separate BlocBuilderListener could get confusing.

@narcodico
Copy link
Contributor

I definitely think that the listener should be added to the BlocBuilder or BlocView, whatever you prefer as a name. You would only have to differentiate the build/listen condition, which could be two separate params like buildWhen and listenWhen. A lot cleaner approach and extremely helpful overall.

@felangel
Copy link
Owner

felangel commented Nov 7, 2019

@RollyPeres but you’d also still have a separate BlocListener in cases where you only want to listen?

@narcodico
Copy link
Contributor

narcodico commented Nov 7, 2019

That or make the builder optional. e.g.: assert(builder != null || listener != null). But that would probably require a name change to something more generic not strictly tied to building, maybe BlocView or something.

@felangel
Copy link
Owner

felangel commented Nov 7, 2019

BlocView still implies there is a view so it also wouldn’t make sense to have an optional builder imo. I personally don’t want to over complicate and muddy the role of BlocBuilder which is why I’ve been hesitant to introduce this. The benefit of introducing this is not that significant imo. Thoughts?

@narcodico
Copy link
Contributor

narcodico commented Nov 7, 2019

I agree in regards to naming, I just threw in the name which @bigworld12 came up with for the sake of example. Well, I also understand there are many of your users already comfortable with the current state of this library. The smartest way to go about this would probably be to keep those two widgets as they are and introduce a new one which combines the two together and let users pick and choose ✌

@felangel
Copy link
Owner

felangel commented Nov 7, 2019

Yeah that’s what I was leaning toward as well but not sure what a good, obvious name would be 🤔

@narcodico
Copy link
Contributor

BlocConsumer ? RIP Remi's naming conventions 👍

@felangel
Copy link
Owner

felangel commented Nov 7, 2019

That could work haha. I’ll think about it some more and discuss with the team 👍

@cgaaf
Copy link

cgaaf commented Nov 7, 2019

Just a few additional names to consider are BlocReceiver and BlocRetriever.

I think also linguistically it feels like something that Provides aka a "BlocProvider" should be matched with something that receives or retrieves. Once a Bloc is received or retrieved, anything could be done with it such as building a widget or listening to the bloc to perform another task. This naming convention could potentially make it easier to add additional functionality in the future through the use of optional parameters.

A consumer linguistically feels more opposite to a "producer" rather than a "provider." BlocConsumer however may fit already more familiar naming conventions with Dart/Flutter.

I also considered BlocClient but that seems a little less intuitive to me as something that receives from a BlocProvider. It feels more appropriate that a BlocClient would be opposite to a BlocServer to fit more traditional naming conventions.

@narcodico
Copy link
Contributor

Since bloc has a small flavor of redux, I'll also nominate BlocConnector.

@felangel felangel added dependency This issue has an external dependency and removed dependency This issue has an external dependency labels Nov 30, 2019
@hawkinsjb1
Copy link

hawkinsjb1 commented Dec 19, 2019

I think just BlocWidget makes sense as far as BLoC pattern goes, b-logic classes represented by Bloc<Event, State> paired with a BlocWidget that presents the interface

@felangel felangel added this to the v3.1.0 milestone Dec 27, 2019
@felangel felangel moved this from To do to In progress in bloc Dec 28, 2019
@felangel felangel added enhancement New feature or request and removed enhancement candidate Candidate for enhancement but additional research is needed question Further information is requested labels Dec 28, 2019
@felangel felangel mentioned this issue Dec 31, 2019
3 tasks
@felangel
Copy link
Owner

felangel commented Jan 1, 2020

Implemented in #753 and released in flutter_bloc v3.1.0 🎉

@felangel felangel closed this as completed Jan 1, 2020
bloc automation moved this from In progress to Done Jan 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Open discussion for a specific topic enhancement New feature or request feedback wanted Looking for feedback from the community
Projects
bloc
  
Done
Development

No branches or pull requests

6 participants