Skip to content

Conversation

@hannah-hyj
Copy link
Member

@hannah-hyj hannah-hyj commented Oct 22, 2022

issue: #103551

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.
drawer-recording-2.mov

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Oct 22, 2022
@hannah-hyj hannah-hyj changed the title [draft] M3 navigation drawer Material 3 navigation drawer Nov 1, 2022
@hannah-hyj hannah-hyj marked this pull request as ready for review November 1, 2022 20:01
@yaymalaga
Copy link

yaymalaga commented Nov 2, 2022

Looking good! For the hovering/selection, the shape should also be rounded, as specified in the navigation drawer specs.

@hannah-hyj
Copy link
Member Author

@yaymalaga Thanks, updated inkwell shape
https://user-images.githubusercontent.com/108393416/199583485-06d81d18-9cfa-44d8-8462-07f19b85f602.mov

Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Nice work!

I have a bunch of minor comments and a couple of suggestions about the API below. Thx!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this being made public? Do we need it someplace else?

If we need to have it as a generally available widget, we should move it into its own file and have tests for it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it back to private and add another private one in NavigationDrawer.

Comment on lines 110 to 112
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be written in a more functional way with something like:

int destinationIndex = 0;
Widget _wrapChild(Widget child) {
  if (child is NavigationDrawerDestination) {
    return SelectableAnimatedBuilder(
            duration: const Duration(milliseconds: 500),
            isSelected: destinationIndex == selectedIndex,
            builder: (BuildContext context, Animation<double> animation) {
              return NavigationDestinationInfo(
                index: destinationIndex,
                // ...
        destinationIndex += 1;
    }
    return child;
}

final List<Widget> wrappedChildren = children.map(_wrapChild).toList();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually after testing it I found out that adding " destinationIndex += 1" in "_wrapChild" will make it not working. Maybe because the function is run lazily?

So I will keep the "for" loop to make it work.

@HansMuller
Copy link
Contributor

HansMuller commented Nov 10, 2022

Here's a version of the example that eliminates the need for the NavigationDestinationData class. I don't think that the cost of adding NavigationDestinationData balances the benefit. Hopefully we do not need public NavigationDestinationInfo or SelectableAnimatedBuilder classes either.

class ExampleDestination {
  const ExampleDestination(this.label, this.icon, this.selectedIcon);

  final String label;
  final IconData icon;
  final IconData selectedIcon;
}

const List<ExampleDestination> destinations = <ExampleDestination>[
  ExampleDestination('page 0', Icon(Icons.widgets_outlined), Icon(Icons.widgets)),
  ExampleDestination('page 1', Icon(Icons.format_paint_outlined), Icon(Icons.format_paint)),
  ExampleDestination('page 2', Icon(Icons.invert_text_snippet_outlined), Icon(Icons.text_snippet)),
  ExampleDestination('page 3', Icon(Icons.invert_colors_on_outlined), Icon(Icons.opacity)),
];

void main() {
  runApp(
    MaterialApp(
      title: 'NavigationDrawer Example',
      debugShowCheckedModeBanner: false,
      theme: ThemeData(useMaterial3: true),
      home: const NavigationDrawerExample(),
    ),
  );
}

class NavigationDrawerExample extends StatefulWidget {
  const NavigationDrawerExample({super.key});

  @override
  State<NavigationDrawerExample> createState() => _NavigationDrawerExampleState();
}

class _NavigationDrawerExampleState extends State<NavigationDrawerExample> {
  final GlobalKey<ScaffoldState> scaffoldKey = GlobalKey<ScaffoldState>();

  int screenIndex = 0;
  late bool showNavigationDrawer;

  void handleScreenChanged(int selectedScreen) {
    setState(() {
      screenIndex = selectedScreen;
    });
  }

  void openDrawer() {
    scaffoldKey.currentState!.openEndDrawer();
  }

  Widget buildBottomBarScaffold(){
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.spaceEvenly,
          children: <Widget>[
            Text('Page Index =  $screenIndex'),
          ],
        ),
      ),
      bottomNavigationBar: NavigationBar(
        selectedIndex: screenIndex,
        onDestinationSelected: (int index) {
          setState(() {
            screenIndex = index;
          });
        },
        destinations: destinations
          .map((ExampleDestination destination) {
            return NavigationBarDestination(
              label: destination.label,
              icon: destination.icon,
              selectedIcon: destination.selectedIcon,
              tooltip: data.label,
            );
          })
          .toList(),
      ),
    );
  }

  Widget buildDrawerScaffold(BuildContext context){
    return Scaffold(
      key: scaffoldKey,
      body: SafeArea(
        bottom: false,
        top: false,
        child: Row(
          children: <Widget>[
            Padding(
              padding: const EdgeInsets.symmetric(horizontal: 5),
              child: NavigationRail(
                minWidth: 50,
                destinations: destinations
                  .map((ExampleDestination destination) {
                    return NavigationRailDestination(
                      label: Text(destination.label),
                      icon: destination.icon,
                      selectedIcon: destination.selectedIcon,
                    );
                  })
                  .toList(),
                selectedIndex: screenIndex,
                useIndicator: true,
                onDestinationSelected: (int index) {
                  setState(() {
                    screenIndex = index;
                  });
                },
              ),
            ),
            const VerticalDivider(thickness: 1, width: 1),
            Expanded(
              child: Column(
                mainAxisAlignment: MainAxisAlignment.spaceEvenly,
                children: <Widget>[
                  Text('Page Index =  $screenIndex'),
                  ElevatedButton(
                    onPressed: openDrawer,
                    child: const Text('Open Drawer'),
                  ),
                ],
              ),
            ),
          ],
        ),
      ),
      endDrawer: NavigationDrawer(
        onDestinationSelected: handleScreenChanged,
        selectedIndex: screenIndex,
        children: <Widget>[
          Padding(
            padding: const EdgeInsets.fromLTRB(28, 16, 16, 10),
            child: Text(
              'Header',
              style: Theme.of(context).textTheme.titleSmall,
            ),
          ),
          destinations
            .map((ExampleDestination destination) {
              return NavigationDrawerDestination(
                label: destination.label,
                icon: destination.icon,
                selectedIcon: destination.selectedIcon,
              );
            }),
          const Padding(
            padding: EdgeInsets.fromLTRB(28, 16, 28, 10),
            child: Divider(),
          ),
        ],
      ),
    );
  }

  @override
  void didChangeDependencies() {
    super.didChangeDependencies();
    showNavigationDrawer =  MediaQuery.of(context).size.width >= 450;
  }

  @override
  Widget build(BuildContext context) {
    return showNavigationDrawer ? buildDrawerScaffold(context) : buildBottomBarScaffold();
  }
}

@HansMuller
Copy link
Contributor

The hover/selection highlights for NavigationDrawerItems in the video below the description are rectangular, but they should be stadium shaped, same as the items themselves.

@hannah-hyj
Copy link
Member Author

hannah-hyj commented Nov 14, 2022

@HansMuller Thanks, I updated the example and updated the video in description

@hannah-hyj hannah-hyj force-pushed the M3-drawer branch 2 times, most recently from 59ca958 to a579c6b Compare November 16, 2022 03:16
drawer

add token

Update navigation_drawer.dart

Update navigation_drawer.dart

Update navigation_rail.dart

navigation_destination_Data

add example

update drawer

lint

update example

update animation

update comments

update comments

add tests

update comments

lint

lint

lint

inkwell shape

lint

lint

fix some tests

Update packages/flutter/lib/src/material/navigation_drawer.dart

Update packages/flutter/lib/src/material/navigation_drawer.dart

Update packages/flutter/lib/src/material/navigation_drawer.dart

Update packages/flutter/lib/src/material/navigation_drawer.dart

Update packages/flutter/lib/src/material/navigation_drawer.dart

Update packages/flutter/lib/src/material/navigation_drawer.dart

Update packages/flutter/lib/src/material/navigation_drawer.dart

apply fix

Update navigation_drawer.0.dart

apply fix

shadowColor

Update packages/flutter/lib/src/material/navigation_drawer.dart

Update packages/flutter/lib/src/material/navigation_drawer.dart

Update packages/flutter/lib/src/material/navigation_drawer.dart

Update packages/flutter/lib/src/material/navigation_drawer.dart

Update packages/flutter/lib/src/material/navigation_drawer.dart

Update navigation_drawer.dart

update example

comment

remove NavigationDestinationData

Add label widget

update label to widget

lint

lint

Add drawerControllerScope

update template

small fix

lint

update test

lint

lint

Update drawer_theme.dart

lint

Update navigation_drawer.dart

Update theme_data_test.dart

Co-Authored-By: Darren Austin <[email protected]>
Co-Authored-By: Hans Muller <[email protected]>
Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Getting close. I just have a few suggestions and questions below.

Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flutter-lgtm

@hannah-hyj hannah-hyj mentioned this pull request Nov 18, 2022
8 tasks
@hannah-hyj hannah-hyj closed this Nov 18, 2022
@hannah-hyj
Copy link
Member Author

I created a new PR from the patch of this one to bypass the CLA check: #115668

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants