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
Support Material 3 in bottom sheet #112466
Conversation
9a027ae
to
c90f531
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @hangyujin. Just have a few suggestions and a question about the M2 defaults. Thx.
@@ -177,6 +177,12 @@ abstract class TokenTemplate { | |||
if (topLeft == topRight && topLeft == bottomLeft && topLeft == bottomRight) { | |||
return '${prefix}RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular($topLeft)))'; | |||
} | |||
if (topLeft == topRight && bottomLeft == bottomRight) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice optimization.
class MyApp extends StatelessWidget { | ||
const MyApp({super.key}); | ||
|
||
static const String _title = 'Flutter Code Sample'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just inline this and make it something like: 'Bottom Sheet Sample'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -526,10 +529,11 @@ class _ModalBottomSheetRoute<T> extends PopupRoute<T> { | |||
child: Builder( | |||
builder: (BuildContext context) { | |||
final BottomSheetThemeData sheetTheme = Theme.of(context).bottomSheetTheme; | |||
final BottomSheetThemeData defaults = Theme.of(context).useMaterial3 ? _BottomSheetDefaultsM3(context) : const BottomSheetThemeData(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the M2 defaults really just an empty BottomSheetThemeData? I am surprised we didn't have some static constants for some of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I didn't find any default value for M2
: super( | ||
elevation: 1.0, | ||
modalElevation: 1.0, | ||
shape: const RoundedRectangleBorder(borderRadius: BorderRadius.vertical(top: Radius.circular(28.0),)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The generated extra comma here would be nice to get rid of. Kind of a pain to do with the template code, but it would look better here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Co-authored-by: Darren Austin <darren@darrenaustin.org>
6363149
to
cc18ec9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
issue: #111448, #111688
fixes #111448
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.