Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Revert attribute content and add assert with pane attribute #531

Merged
merged 8 commits into from
Sep 20, 2022

Conversation

WinXaito
Copy link
Collaborator

@WinXaito WinXaito commented Sep 14, 2022

Pre-launch Checklist

  • I have updated CHANGELOG.md with my changes
  • I have run "dart format ." on the project
  • I have added/updated relevant documentation

I have some case where I use the navigation view like this:

return NavigationView(
  content: ScaffoldPage(
    header: PageHeader(
      title: titleRow,
    ),
    content: Padding(
      padding: const EdgeInsets.symmetric(horizontal: 25),
      child: child,
    ),
  ),
);

And after the PR #510 this is not possible, we need to do something like that:

```dart
return NavigationView(
  pane: NavigationPane(
      displayMode: PaneDisplayMode.minimal,
      items: [
        PaneItem(
          icon: const SizedBox.shrink(),
          body: ScaffoldPage(
            header: PageHeader(
              title: titleRow,
            ),
            content: Padding(
              padding: const EdgeInsets.symmetric(horizontal: 25),
              child: child,
            ),
          ),
        )
      ]
  ),
);

With this fix, we can use one solution with pane or content (but one of these must be null !).

@WinXaito WinXaito requested a review from bdlukaa September 14, 2022 07:40
@WinXaito WinXaito changed the title Revert attribute content and add assert with panes attribute Revert attribute content and add assert with pane attribute Sep 14, 2022
@WinXaito WinXaito requested a review from bdlukaa September 19, 2022 06:15
@bdlukaa bdlukaa merged commit 6de655d into bdlukaa:master Sep 20, 2022
@WinXaito WinXaito deleted the fix_navigationview branch September 20, 2022 17:07
@henry2man
Copy link
Contributor

Prior to 4.0.0 stable I had both PaneItems , a NavigationPane.onChanged simple callback and a LazyIndexedStack in content attribute:

var index = 0;
...
NavigationView ( 
 content: LazyIndexedStack(
    index: index,
    children: [
                  (context) => const ScreenOne(),
                  (context) => const ScreenTwo(),
                  (context) => const ScreenThree(),
    ]),
pane: NavigationPane(
                    selected: index,
                    onChanged: (i) {
                      setState(() => index = i);
                    },
                    items: [
                      PaneItem(
                        icon: Icon(
                          Icons.home,
                        ),
                        title: Text("One"),
                      ),
                      PaneItem(
                        icon: Icon(
                          Icons.home,
                        ),
                        title: Text("Two"),
                      ),
                      PaneItem(
                        icon: Icon(
                          Icons.home,
                        ),
                        title: Text("Three"),
                      ),

With this setup I was able to maintain internal state for every page in the LazyIndexedStack. But since I've updated to +4.0.0 stable I had to decide between PaneItem + body or not having PaneItems at all. I could go for PaneItem + body if there were a mechanism to preserve state, but until now LazyIndexedStack was the only option for me.

@bdlukaa
Copy link
Owner

bdlukaa commented Oct 31, 2022

you can use a NavigationView.paneBodyBuilder

@henry2man
Copy link
Contributor

Sadly, NavigationView.paneBodyBuilder doesn't works (even with keys) because on every PaneItem page switch the state is rebuilt.

I've opened #595 in order to allow current behavior and pre 4.0.0 stable behavior, too

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants