Updated Theming Cookbook for M3#9183
Conversation
atsansone
requested review from
domesticmouse,
johnpryan,
khanhnwin,
parlough and
sfshaza2
as code owners
|
Looks like the code snippets need refreshing |
sfshaza2
left a comment
sfshaza2
left a comment
There was a problem hiding this comment.
So, the general rule of thumb for codefonting classes, is that if you use it as a regular word, use lowercase and no codefont. If you really mean the specific class as a name, uppercase and codefont it. I'm not 100% sure if this is covered in the GDSG, but it's what we've always done. Not just here, but in Java docs, too. :)
Also, always indent the text for a note by two spaces on each line. I believe that's documented in our site shared docs? (But I'm not 100% sure.)
atsansone
force-pushed
the
fix-7341
branch
3 times, most recently
from
8cd662a to
c9d8e1a
Compare
| // section on `Theme.of`. | ||
| data: Theme.of(context) | ||
| .copyWith(colorScheme: ColorScheme.fromSeed(seedColor: Colors.pink) | ||
| // TRY THIS: Change the seedColor to "Colors.red" or |
There was a problem hiding this comment.
Nit: Move this comment to the line before 61. Comments usually refer to the current line or the following line, not the previous line.
There was a problem hiding this comment.
reply: Usually, I would agree. I want this to come after as it is optional and I want the reader to see the default before another option is presented.
There was a problem hiding this comment.
In the examples, this pattern usually does have the comment come before the thing that could be changed. People often run the example before inspecting the code carefully, so in practice they will often be reading the comment after they've run it once anyhow.
gspencergoog
left a comment
gspencergoog
left a comment
There was a problem hiding this comment.
In general, make a pass to make sure your comments are complete sentences with punctuation (esp. periods at the end).
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use-correct-grammar
gspencergoog
left a comment
gspencergoog
left a comment
There was a problem hiding this comment.
This looks pretty good. Once the README.md is updated, I think it will be good to go.
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| final theme = Theme.of(context); // Simplifies later use of Theme context. |
There was a problem hiding this comment.
nit: I'm not sure "Theme context" is a phrasing we use elsewhere. It's accurate, but "context" is such an overloaded term in Flutter docs that I tend to avoid using its normal literal meaning and pick a synonym. 😄
Maybe "theme data" would work here?
| // Find and extend the parent theme using `copyWith`. | ||
| // To learn more, check out the next | ||
| // section on `Theme.of`. |
There was a problem hiding this comment.
nit: are the line breaks in this comment correct?
src/cookbook/design/themes.md
Outdated
| This property takes a takes a [`ThemeData`][] widget. | ||
|
|
||
| To enable Material 3, add the [`useMaterial3`][] property | ||
| to the `ThemeData` widget. |
There was a problem hiding this comment.
nit: "To enable Material 3, set ThemeData's useMaterial3 property to true."
Technically, the coder wouldn't be adding a property to ThemeData -- the property is part of the class definition. They need to assign a value to the property on the instance of ThemeData that they're using.
src/cookbook/design/themes.md
Outdated
|
|
||
| See the [`ThemeData`][] documentation to see all of | ||
| the colors and fonts you can define. | ||
| Most `ThemeData` objects include two properties. |
There was a problem hiding this comment.
Related to the comment above, technically speaking all ThemeData objects have those two properties -- they just aren't always specifically assigned by the app developer when calling the constructor.
If this sentence is intended to make the point that textTheme and colorScheme are two of the most important and commonly used properties of ThemeData, that's a great thing to include. Consider just calling it out directly that way.
| // Define the default font family. | ||
| fontFamily: 'Georgia', | ||
| // Define the default brightness and colors. | ||
| colorScheme: ColorScheme.fromSwatch( |
There was a problem hiding this comment.
We don't recommend using fromSwatch (M1/M2 concept) but rather fromSeed (M3+)
There was a problem hiding this comment.
It looks like this isn't documented in the API, filed flutter/flutter#132584 Thanks! :)
There was a problem hiding this comment.
When I try them in DartPad, the results are not very predictable and may need further testing. I would like to address this as a separate PR. Other than this, can you approve? I'm happy to make another issue and PR to make these changes.
There was a problem hiding this comment.
Sure, can you file an issue so this doesn't get lost (or modify @Piinks' issue?
There was a problem hiding this comment.
My feeling is that this might be worth addressing before landing the change, as with the current state of the PR, the recipe now uses a mix of fromSwatch and fromSeed. That might just contribute to further confusion for such a core theming concept.
| useMaterial3: true, | ||
|
|
||
| // Define the default brightness and colors. | ||
| colorScheme: ColorScheme.fromSwatch( |
| // Create a unique theme with `ThemeData`. | ||
| data: ThemeData( | ||
| splashColor: Colors.yellow, | ||
| primarySwatch: Colors.pink, |
src/cookbook/design/themes.md
Outdated
| If a widget doesn't use a specific theme, the visual properties fall back | ||
| to the main theme. Precedence in themes rank from the specific widget, | ||
| to the widget type theme, to the general theme. |
There was a problem hiding this comment.
Not super clear
| If a widget doesn't use a specific theme, the visual properties fall back | |
| to the main theme. Precedence in themes rank from the specific widget, | |
| to the widget type theme, to the general theme. | |
| Typically, visual properties are resolved first with the | |
| widget properties (e.g. `AppBar.centerTitle`). If not specified, then the widget theme (e.g. `AppBarTheme.centerTitle` of the ambient theme) is used. If not specified, then some | |
| reasonable default is used. |
There was a problem hiding this comment.
@atsansone I also think it's important to handle this clarification before this lands. It's not clear what "If a widget doesn't use a specific theme" means and the existing text doesn't account for the properties on the widgets themselves as @guidezpl highlighted in his suggestion.
src/cookbook/design/themes.md
Outdated
| // Define the default font family. | ||
| fontFamily: 'Georgia', | ||
| // Define the default brightness and colors. | ||
| colorScheme: ColorScheme.fromSwatch( |
src/cookbook/design/themes.md
Outdated
| // Create a unique theme with `ThemeData`. | ||
| data: ThemeData( | ||
| splashColor: Colors.yellow, | ||
| primarySwatch: Colors.pink, |
src/cookbook/design/themes.md
Outdated
| fontFamily: 'Georgia', | ||
| // Define the default brightness and colors. | ||
| colorScheme: ColorScheme.fromSwatch( | ||
| primarySwatch: Colors.purple, |
|
@atsansone, I need to see this staged. thx |
parlough
left a comment
parlough
left a comment
There was a problem hiding this comment.
Thanks for making those adjustments! A few more comments on those changes:
| - How to override the overall theme's color and font values for | ||
| one or more components in a Flutter app. | ||
|
|
||
| ## Source Code |
There was a problem hiding this comment.
Thanks for adding the link to the document!
The following link isn't the source code, so consider a different title. Maybe something like:
| ## Source Code | |
| ## Live document |
src/cookbook/design/themes.md
Outdated
| If you have a standalone `Theme` as defined in the previous example, | ||
| that's applied. |
There was a problem hiding this comment.
What previous example is this talking about? Sounds like it's talking about the sample shown in the next step.
parlough
added
review.await-update
parlough
added
st.RFM.%
|
@parlough : I can't merge without an explicit approval. Can you provide one? |
parlough
left a comment
parlough
left a comment
There was a problem hiding this comment.
Sure! Please address my recent suggestions in #9183 (review) before merging.
Thanks so much for updating and improving this important recipe.
Co-authored-by: Parker Lougheed <[email protected]>
Fixes flutter#7729 Fixes flutter#7341 Staged: https://tony-flutter-site--fix-4371-k2clzujr.web.app/cookbook/design/themes --------- Co-authored-by: Shams Zakhour (ignore Sfshaza) <[email protected]> Co-authored-by: Greg Spencer <[email protected]> Co-authored-by: Parker Lougheed <[email protected]>
10 participants
Fixes #7729
Fixes #7341
Staged: https://tony-flutter-site--fix-4371-k2clzujr.web.app/cookbook/design/themes