Improve useMaterial3 documentation
#104815
Conversation
flutter-dashboard
bot
added
f: material design
|
For reference, this is what it looks like rendered |
| /// * [Typography](https://m3.material.io/styles/typography): `typography` (see table below) | ||
| /// | ||
| /// ## Components | ||
| /// * [Common buttons](https://m3.material.io/components/buttons): [TextButton], [OutlinedButton], [ElevatedButton] |
There was a problem hiding this comment.
Perhaps, differentiate spec links from API doc links?
- Common buttons: TextButton, OutlinedButton, ElevatedButton (spec)
Or remove spec link:
- Common buttons: TextButton, OutlinedButton, ElevatedButton
There was a problem hiding this comment.
I would opt for leaving the spec link out here. I think most of these have links to the spec in the individual component docs (or should if they don't).
There was a problem hiding this comment.
I added spec links to all API docs
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
The changes look OK to me and I agree that it's just as well if the API doc only links to API docs, not m3.material.io. Other than that, I defer to @darrenaustin
darrenaustin
left a comment
darrenaustin
left a comment
There was a problem hiding this comment.
Thanks for the cleanup. I just had some minor comments and one issue below.
| /// * [Typography](https://m3.material.io/styles/typography): `typography` (see table below) | ||
| /// | ||
| /// ## Components | ||
| /// * [Common buttons](https://m3.material.io/components/buttons): [TextButton], [OutlinedButton], [ElevatedButton] |
There was a problem hiding this comment.
I would opt for leaving the spec link out here. I think most of these have links to the spec in the individual component docs (or should if they don't).
| /// some properties will get special defaults. However, just copying a [ThemeData] | ||
| /// with [useMaterial3] set to true will not change any of these properties in the | ||
| /// resulting [ThemeData]. These properties are: |
There was a problem hiding this comment.
I think we need this description (or one like it here), as this is a distinction that a lot of people will miss and be confused when they don't get all the M3 features. A lot of apps tend to create a ThemeData with some default constructor like ThemeData.light() and then use .copyWith to configure it. If they do that with this property they won't get the new typography or splash factory defaults.
I would also like to see this above the component list as this is easily missed here at the bottom of the doc.
There was a problem hiding this comment.
Hmm I think that's a bug then. I would expect copyWith with useMaterial3 true to apply these updates, if I haven't provided my own typography and splashFactory, of course.
If I understand correctly, we're currently requiring every created theme out there (that doesn't use the factory constructor) to add the following?
...
typography: Typography.material2021(platform: platform),
splashFactory: platform == TargetPlatform.android && !kIsWeb ? InkSparkle.splashFactory ? InkRipple.splashFactory,
)
It's not documented anywhere and is pretty brittle.
Nevertheless, that's out of scope for this PR, so I've moved this section above the styles and components sections.
There was a problem hiding this comment.
Hmm I think that's a bug then. I would expect
copyWithwithuseMaterial3true to apply these updates, if I haven't provided my owntypographyandsplashFactory, of course.
Not a bug so much as an easily misunderstood feature. copyWith just blindly overrides any given parameters, but does no other processing. Even if we want to special case this for useMaterial3, once a ThemeData is constructed, it will have non-null typography and splashFactory settings. We would not be able to tell if you have provided your own or if they were default.
If I understand correctly, we're currently requiring every created theme out there (that doesn't use the factory constructor) to add the following?
... typography: Typography.material2021(platform: platform), splashFactory: platform == TargetPlatform.android && !kIsWeb ? InkSparkle.splashFactory ? InkRipple.splashFactory, )
There is no way to construct a ThemeData that doesn't use the factory constructor other than ThemeData.raw or ThemeData.copyWith (which itself uses .raw). Unless I am missing something here.
If people want to use copyWith in conjunction with useMaterial3 they will get most of the features, just not the ones that have defaults set in the factory constructor (hence the description in the docs).
Nevertheless, that's out of scope for this PR, so I've moved this section above the styles and components sections.
Thanks.
flutter-dashboard
bot
added
the
a: text input
darrenaustin
left a comment
darrenaustin
left a comment
There was a problem hiding this comment.
Sorry for the lag on my review. LGTM. Thx.
* Improve `useMaterial3` documentation * reverse styles and components * tweak language around support * Update theme_data.dart * remove trailing space * Update packages/flutter/lib/src/material/theme_data.dart Co-authored-by: Darren Austin <[email protected]> * add missing spec links * remove spec links from useMaterial3 * move defaults section up, rejig things * spaces no longer trailing * spaces no longer trailing (2) Co-authored-by: Darren Austin <[email protected]>
3 participants
This PR also adds links to relevant spec to all currently migrated widgets.
#91605
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.