Expose mainContentSlivers's enclosing context

[vishna]

What feature would you like to see?

Using one of the state management libs (bloc) currently it's not possible to e.g. refresh item count of items on the list.

Given the following use of decorator

WoltModalSheet.show(
  context: context,
  pageListBuilder: (ctx) => [sheet(ctx)],
  decorator: (widget) {
    return BlocProvider(
      create: (_) => MyCubit(),
      child: widget,
    );
  },
);

I am not able to refresh contents of mainContentSlivers list

sheet(
  BuildContext context,
) {
  final items = context.watch<MyCubit>().state; // doesn't trigger rebuild
  return SliverWoltModalSheetPage(
    mainContentSlivers: [
      SliverList.builder(
        itemBuilder: (context, index) {
          return Text(items[index]);
        },
        itemCount: items.length,
      ),
    ];
  );
}

Attaching a PR where I fixed the problem for myself (needs API decision from project owners tho)

sheet(
  BuildContext context,
) {
  return SliverWoltModalSheetPage(
    mainContentSlivers: [], // here just not to break anything
    mainContentSliversBuilder: (context) {
      final items = context.watch<MyCubit>().state; // works
      return [
        SliverList.builder(
          itemBuilder: (context, index) {
            return Text(items[index]);
          },
          itemCount: items.length,
        ),
      ];
    },
  );
}

[vishna]

The PR here: #177

[ulusoyca]

Hi @vishna We had similar issue in our codebase and this is how we resolved it:

Would something like this work in your case?

[ulusoyca]

We have an ongoing work to use WoltModalSheetPage as a widget. This will solve problems like this. ping @MbIXjkee

[vishna]

Hi @ulusoyca 👋 In the example you posted, you use WoltModalSheetPage which is simply a wrapper around SliverToBoxAdapter ...which prevents you from using e.g. SliverList.builder - which is something you would want for larger/dynamic lists.

It might be worth changing mainContentSlivers from List<Widget> to List<Widget> Function(BuildContext) but that would be a breaking change 💥 Anyway, curious to hear how you guys plan to rework WoltModalSheetPage 👀

[ulusoyca]

@vishna This is the sneak peak PR about the changes: #149

...which prevents you from using e.g. SliverList.builder - which is something you would want for larger/dynamic lists.

In our use case we don't have large list, and this is why we went with this solution. However, in the example the ListenableBuilder approach should also work for mainContentSlivers. Have you tried it?

[ulusoyca]

I see your point :) mainContentSlivers is not a widget 🤦🏻 @TahaTesser How can we improve the API? Can you check the proposal?

[vishna]

I see your point :) mainContentSlivers is not a widget 🤦🏻 @TahaTesser How can we improve the API? Can you check the proposal?

Exactly 😅 I wish there was something like:

SliverBuilder((context) {
   return someSliver;
});

that would solve the issue without API changes 🙄

[TahaTesser]

• We could make mainContentSlivers property nullable and remove required. This shouldn't break existing code as this property will be already used.
• Either mainContentSlivers or mainContentSliversBuilder must be provided. This also doesn't break the required slivers rule, one of them will be provided. Only if the user wanted they can switch new mainContentSliversBuilder property.
• Cannot use both mainContentSlivers and mainContentSliversBuilder at the same time. We would need only one of them.

@ulusoyca if we had complete test coverage we could run them to verify such cases aren't breaking anything. However, this should be relatively safe change.

[TahaTesser]

(I don't usually post screenshots of code but this is just for the proposal)

Here you can see new builder type, nullable mainContentSlivers and assertions to safeguard incorrect usage.

[ulusoyca]

This is amazing @TahaTesser. LGTM.

[ulusoyca]

@TahaTesser why don't we introduce a breaking change? The SliverModalSheetPage is rarely used, and we are still far from 1.0, so breaking changes as the API evolves is inevitable. I think instead of keeping multiple fields, I would prefer having only mainContentSliversBuilder to avoid confusion. WDYT?

[vishna]

You could also keep mainContentSlivers for a while but mark it deprecated, and schedule for removal in e.g. 0.6.x or something like that 🤷

[ulusoyca]

@vishna Yes, probably I should be a nice guy :) Power comes with responsibility. Thanks, a very good idea.

[TahaTesser]

@TahaTesser why don't we introduce a breaking change? The SliverModalSheetPage is rarely used, and we are still far from 1.0, so breaking changes as the API evolves is inevitable. I think instead of keeping multiple fields, I would prefer having only mainContentSliversBuilder to avoid confusion. WDYT?

If the goal is to remove it, then yes, we can make it nullable as well as mark it deprecated. This will give enough time for the user to migrate too.

This way the user doesn't have to provide an empty list.

[dickermoshe]

If you don't mind, I had another thought while using this package myself.
This package is very rigid in how you implement it. It's API is very different from everything else found in flutter.

1. Even with a builder on sliver, no other parts of the page SliverWoltModalSheetPage reactive.

return SliverWoltModalSheetPage(
      leadingNavBarWidget: IconButton(
          icon: const Icon(Icons.close),
          onPressed: () {
            Navigator.of(context).pop(false);
          }),
      sabGradientColor: , // No way to edit this after build)
}

Why am I doing reactively in a function, when I should be doing it in the build method of widget state.

2. Reuse Sheets
   A product detail sheet, if there is a "similar product section" you would need to do the following

pageListBuilder: (context) {
  return [
    buildProductPage(product1), // Main Product
    buildProductPage(product2), // Similar Product 1
    buildProductPage(product3), // Similar Product 2
    buildProductPage(product4), // Similar Product 3
    ...
  ];
},

You would then need to devise some complex logic so that the main product page knows the index for each of the similar products.
If the similar products are only known after the sheet is built, there is no way add them later.
You can't push a route to the stack with parameters, they are static.

3. State Management
   You can't pop with data either, meaning that you can't return a result from one page to another, you must use a value notifier. Being forced to use Bloc/Riverpod for passing state around ain't great.
   Even if you had a builder for the main sliver body, there are other widgets in SliverWoltModalSheetPage, that all need to be ValueListenableBuilder (topBarTitle, leadingNavBarWidget, etc.). So state for each of the pages wind up in a single Value Notifier. It's the only way to change the SAB by tapping something in the sliverBody for example.

final page1State = ValueNotifier(PageOneState); // Custom object to hold state
final page2State = ValueNotifier(PageTwoState); // Custom object to hold state
...
WoltModalSheet.show(
pageListBuilder: (context) {
    return [
      buildProductPage(product1,page1State ),
      buildProductPage(product2,page2State ),
    ];
  },
)

Forgive my ramblings, I may have no idea how to use this package, but I would imagine something like this:

// Sheet is a page route that can be extending with parameters

class Sheet extends Wolt...{
   Sheet(this.product)
}

// Add sheet
WoltModalSheet.of(context).push(Sheet(product1))
WoltModalSheet.of(context).pop(true)

[dickermoshe]

We have an ongoing work to use WoltModalSheetPage as a widget. This will solve problems like this.

This seems like the way to go, now it's a cross between a route (for the page) and a widget (for each component), and it feels like everything I'm doing is a workaround. (Putting local state into global state management).

I may be coming off as angry or upset. Forgive me. Thank so much for this free and awesome package!

[ulusoyca]

No worries at all! We are open to feedback. We did not create this project to receive kudos. Constructive feedback helps us to be better everyday.

Even with a builder on sliver, no other parts of the page SliverWoltModalSheetPage reactive.

The problem is fundamental. We are aware of the fact that WoltModalSheetPage should be a widget. We acknowledge this was a mistake, but this was the easiest way at the time to deliver a solution according to the design specs.

The rest of the components is different than mainContentSlivers because it is a list of widgets, not a single widget. For the rest, we are utilizing decorator, ListenableBuilder, ValueListenableBuilder etc.

You can't push a route to the stack with parameters, they are static.

In the sample app there is a demo of how to do it. The showWithDynamicPath (here) static method allows setting the paths dynamically. This can be useful to dynamically create a page stack. In the end, there is only one page. Mostly two pages during the transition, and after the pagination transition is complete, we demount the outgoing page from the tree.

You can't pop with data either

This should be the same behavior as the bottom sheet usage in Flutter SDK. It returns data, and in fact we are returning data in our project when popped imperatively. Navigator.of(context).pop<T>(result);

Even if you had a builder for the main sliver body, there are other widgets in SliverWoltModalSheetPage, that all need to be
ValueListenableBuilder (topBarTitle, leadingNavBarWidget, etc.). So state for each of the pages wind up in a single Value Notifier. It's the only way to change the SAB by tapping something in the sliverBody for example.

This is true, until the WoltModalSheet is a widget. I agree it is not nice, but for now, this works.

          stickyActionBar: ValueListenableBuilder(
            valueListenable: actionTextEnabled,
            builder: (_, isEnabled, ___) => LargeButton(
              text: strings.orderHistory_modal_action,
              onPressed: () {
                Navigator.pop(context, selectedOptionValue);
              },
              disabled: !isEnabled,
            ),
          ),

Forgive my ramblings, I may have no idea how to use this package, but I would imagine something like this
This is a good API, and I must admit I never thought of this. I will keep this in mind.

Summary: All of your input is valuable and valid. At Wolt, we are in the finalization of the migration process from native iOS merchant app to Flutter merchant app that affects many users. After the migration is complete, we will have more time and freedom to focus on these changes for the package. This is why the package for now stays in 0.y.z version.

[dickermoshe]

@ulusoyca

In the sample app there is a demo of how to do it. The showWithDynamicPath (here) static method allows setting the paths dynamically. This can be useful to dynamically create a page stack. In the end, there is only one page. Mostly two pages during the transition, and after the pagination transition is complete, we demount the outgoing page from the tree.

Problem

I've began writing a controller for this package that would simplify many parts of it.
However this throws an error:

_pageListBuilderNotifier.value = (BuildContext context) {
      return [currentPage, page];
};
_pageIndexNotifier.value = 1;

RangeError (index): Invalid value: Only valid value is 0: 1

Seems to be an issue with the sheet using the old pageListBuilder
Wrapping _pageIndexNotifier.value = 1; in a future with Duration.zero, doesn't help. It needs a delay of at least 10 milliseconds to avoid this error.

_pageIndexNotifier.value = 1; // Error

Future.delayed(Duration.zero, () => _pageIndexNotifier.value = 1); // Error

Future.delayed(Duration(milliseconds: 10), () => _pageIndexNotifier.value = 1); // Works

Diagnose

The interesting thing is that the ValueListenableBuilder for pagesListBuilderNotifier is not throwing an error.
This runs fine and pages get's the correct page

wolt_modal_sheet/lib/src/wolt_modal_sheet.dart

Lines 220 to 226 in 7b5572a

	ValueListenableBuilder<WoltModalSheetPageListBuilder>(
	valueListenable: pagesListBuilderNotifier,
	builder: (context, pagesBuilder, _) {
	final pages = pagesBuilder(context);
	assert(pages.isNotEmpty,
	'pageListBuilder must return a non-empty list.');
	return ValueListenableBuilder<int>(

The error is being thrown by the animation segment of the code:

wolt_modal_sheet/lib/src/content/wolt_modal_sheet_animated_switcher.dart

Line 355 in 7b5572a

_incomingOffstagedMainContentKeys[_pageIndex],

It seems that _incomingOffstagedMainContentKeys still has the old pages.

I was going to post an issue but remembered you said

Mostly two pages during the transition, and after the pagination transition is complete, we demount the outgoing page from the tree.

Am I doing this wrong, or is this a bug, (in my code most probably) ?

[ulusoyca]

I will get back to this, but you are not wrong. In my side project, I also wait for the next frame when dynamically setting the path on callback. Need to investigate why.

  static void _goToPage(ValueNotifier<int> pageIndexNotifier, int index) {
    SchedulerBinding.instance.goToModalPageOnNextFrame(pageIndexNotifier, index);
  }

https://twitter.com/i/status/1735743427499921615

[ulusoyca]

@dickermoshe

This is not the case when you set the value in the ValueNotifier for page list builder and then set the value of the valuenotifier of the page index. My theory is this: PageListeBuilder notifying requires to wait a complete rendering pipeline of a frame before setting the page list notifier.

[ulusoyca]

@dickermoshe I started working on this FYI.

[dickermoshe]

Checkout my PR, It does that.
Btw wrapping each of the sheet segments in a statfulbuilder makes them work with hot reload.
Would be cool if the package did that automaticly under the hood

[dickermoshe]

That's awesome!

[ulusoyca]

@dickermoshe This is my draft proposal. Needs some more cases to test with the playground.
#182

WMS changes: https://github.com/woltapp/wolt_modal_sheet/blob/14ee6fe422b80ce56bcafdbeb5f40880250b8f95/lib/src/wolt_modal_sheet.dart

[ulusoyca]

Merged #177