-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Migrate Dialog to Material 3
#98919
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
Migrate Dialog to Material 3
#98919
Conversation
|
I think this is because the font is somewhat different. The fact that the text box seem to line up with the text of the button is just coincidence. |
|
Is that from material 3? Yeah, fair enough. I think they used the buttons without inner padding in the image. |
Yes its from here: https://m3.material.io/components/dialogs/specs |
|
Thanks. There was a different issue (which I believe they have fixed since then 🎉), but I'm really happy with the current outcome. |
|
@bernaferrari @LasseRosenow I've created a dialog overlay example https://github.com/TahaTesser/M3_Dialog_Overlay example (based on https://github.com/nt4f04uNd/tabs_overlay) (top dialog is native)
I will look into changes needed to match the M3 design. cc: @darrenaustin |
|
[I know this is radical, but..] can't you make the Dialog work like buttons? Something like I don't know what are the plans for Dialog theme, if they refactored already or not. But maybe, MAYBE, that could also be done. |
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.
@TahaTesser thanks for taking the initiative and tackling this migration.
Looks good, with a few suggestions and comments below.
You will probably also want to merge with the latest from master as I updated the token JSON files in #99292 earlier today.
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.
For properties that don't need a computation (like looking up a color from the ColorScheme), you can just include them as a parameter to the super class in the constructor. The main reason we want to override some of the properties with getter methods is so we aren't computing something until it actually needed. In the case of constants we don't need to do this.
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.
Same with these constant properties, just make them parameters to the super class in the constructor.
Not sure I understand what you are asking here. The current plan for migration is to use the component theme data classes to encapsulate as much of the token information as we can. This way if the code just depends on a FooThemeData for defaults it is easy to switch out a M2 theme data for a M3 theme data (as you have done with the DialogTheme). |
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.
Looks good. Just have a comment below that should be updated with a TODO.
Also, can you add Dialog and AlertDialog to the list of components affected by the useMaterial3 flag in doc comment for ThemeData.useMaterial3?
Otherwise, I think we are good to go. Thx!
darrenaustin
left a comment
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.
|
This pull request is not suitable for automatic merging in its current state.
|
35db459 to
85ac2bb
Compare
Update template Kick tests refactor defaults
Kick tests Kick tests
|
Rebasing one more time to update any infra changes in the updated master branch so tests can pass |






Part of: #91605
Updated the
Dialog/AlertDialogwidget with support for Material Design 3.Fixes: #99025
minimal code sample
In order to use the
Dialog/AlertDialogwith the new Material 3 defaults, turn on theuseMaterial3flag in theThemeData:Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.