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

[adaptive_navigation] Add Material 3 support. #509

Closed

Conversation

zbLiuLiu
Copy link

Description

Add Material 3 support. Automatically use NavigationDrawer and NavigationBar while ThemeData.useMaterial3 is true.

Tests

Add theme prop in MaterialApp and set useMaterial3 to true.

Checklist

@zbLiuLiu zbLiuLiu requested a review from a team as a code owner November 28, 2023 12:08
@zbLiuLiu zbLiuLiu requested review from guidezpl and removed request for a team November 28, 2023 12:08
Copy link

google-cla bot commented Nov 28, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@zbLiuLiu zbLiuLiu changed the title Add Material 3 support. [adaptive_navigation] Add Material 3 support. Nov 28, 2023
Copy link
Collaborator

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

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

thanks for contributing! some comments below

Copy link
Collaborator

Choose a reason for hiding this comment

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

please revert this file

@@ -1,3 +1,7 @@
## Unreleased
### Added
- Add Material 3 support.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Add Material 3 support.
- Added Material 3 support

Copy link
Collaborator

Choose a reason for hiding this comment

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

please revert

icon: Icon(destination.icon),
label: Text(destination.title),
),
// const Spacer(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove or uncomment

double elevation = navigation.elevation ??
Theme.of(context).navigationDrawerTheme.elevation ??
1.0;
return Material(
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the need for this Material?

@@ -39,12 +40,12 @@ class AdaptiveScaffoldDestination {
});
}

/// A widget that adapts to the current display size, displaying a [Drawer],
/// [NavigationRail], or [BottomNavigationBar]. Navigation destinations are
/// A widget that adapts to the current display size, displaying a [Drawer]/[NavigationDrawer],
Copy link
Collaborator

Choose a reason for hiding this comment

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

please adopt https://dart.dev/effective-dart/documentation#do-separate-the-first-sentence-of-a-doc-comment-into-its-own-paragraph

also, can you clarify that [NavigationDrawer] is if the ambient [ThemeData.useMaterial3] is true, otherwise [Drawer] is used.

@guidezpl
Copy link
Collaborator

The adaptive packages have been discontinued, therefore I must close this PR. Thanks!

@guidezpl guidezpl closed this Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants