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

☂️ Material Theme System Updates #91772

Open
20 of 40 tasks
Tracked by #91605
guidezpl opened this issue Oct 13, 2021 · 18 comments
Open
20 of 40 tasks
Tracked by #91605

☂️ Material Theme System Updates #91772

guidezpl opened this issue Oct 13, 2021 · 18 comments
Assignees
Labels
c: tech-debt Technical debt, code quality, testing, etc. f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list team-design Owned by Design Languages team triaged-design Triaged by Design Languages team

Comments

@guidezpl
Copy link
Member

guidezpl commented Oct 13, 2021

This issue mirrors the design of https://flutter.dev/go/material-theme-system-updates

Rename the TextTheme TextStyles

Deprecate/remove the accent properties

Compute brightness, primaryColorBrightness, accentColorBrightness

Deprecate/move/remove the 21 grab-bag properties

(x) indicates number of references, excludingtheme_data.dart and tests. This can be used a proxy for disruptiveness/ease of deprecation.

Additional grab-bag properties

These grab-bag properties not in the original linked proposal, but can also be deprecated:

These factory constructor parameters can also be deprecated:

Property colors not subject to removal

  • State colors like ThemeData.disabledColor (26), ThemeData.focusColor (19), and ThemeData.hoverColor (19) have high usage and are shared amongst many components
  • There is similar widespread usage for ThemeData.highlightColor (13)
  • Other parameters can remain in the factory constructor (e.g. ThemeData.fontFamily and ThemeData.fontFamilyFallback) for convenience

Component Theme Normalization

All of the existing themes have been normalized except for the following:

@guidezpl guidezpl mentioned this issue Oct 13, 2021
8 tasks
@danagbemava-nc danagbemava-nc added in triage Presently being triaged by the triage team f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. passed first triage and removed in triage Presently being triaged by the triage team labels Oct 14, 2021
@rydmike
Copy link
Contributor

rydmike commented Nov 10, 2021

Nice summary @guidezpl and great to see this collection issue for moving this topic forward and tracking its progress. It will certainly make things clearer when completed. The current situation seems to be a source of a lot of confusion for many Flutter developers, quite a few related issues open and more arriving ever so often. This will help, eventually.

Most of the colors properties already have proper homes in the ColorScheme and/or appropriate sub-themes, where you can override widget default inherited colors, that properly designed SDK Material UI widgets now get from ThemeData.colorScheme.

For those colors everything looks fine, but not all color properties in ThemeData are so lucky and at that stage. Below I highlight one important case: scaffoldBackgroundColor, there might be others too.

Things to Consider during the Theme System Update and Migration

In case it is not obvious, but as far as I know e.g. ThemeData.scaffoldBackgroundColor currently does not have a home anywhere else. If you do not want it to be same color as ThemeData.colorScheme.background, which presumably would be its new default, ThemeData.from() already assigns this color to it. Then there is no other way to currently theme it.

The only way to set it separately, is to define the ThemeData.scaffoldBackgroundColor, so it needs a new home somewhere.

Being able define slightly different background colors depending on type of surface/background is important. It makes it possible in current version of Flutter to create applications that uses background colors with slightly different nuances, typically with different alpha blend strengths of primary color in the different surface/background colors used by Widgets, depending on their type and current default themed color behavior.

For example, currently with the factory ThemeData.from, that is the most up to date one of the color factories for using proper ColorScheme based themes, we for example have:

  • colorScheme.surface, that is assigned to cardColor, used by Card and Material of type card. Expecting these widgets to use colorScheme.surface directly after migration. This is fine and will not hard break anything, change can imo be done easily, everything looks ready for it.
  • colorScheme.background, that is assigned to canvasColor, used by Material of type canvas, and via that many widgets. Expecting Material of type canvas to use colorScheme.background directly after migration. This is fine and will not hard break anything, change can imo be done easily, everything looks ready for it.
  • dialogBackgroundColor that gets assigned colorScheme.background, already has its own sub-theme, so we can easily theme it to whatever we like (we already use that route in anticipation of dialogBackgroundColor deprecation. This prop can imo be deprecated easily already now, migration path is clear/ready.
  • scaffoldBackgroundColor, is also assigned colorScheme.background, but has no other source that can be used to theme it, other than the current ThemeData. It will need it's own new sub-theme, otherwise scaffoldBackgroundColor cannot be deprecated without reduction in currently used theming capabilities and flexibility. It would break things very badly in our apps if it cannot be themed to another color than ThemeData.colorScheme.background going forward.

This might apply to a few other ThemeData color properties on the deprecation list as well.

PS. Yes, I'm aware Material3 brings new containers used for color and onColor, so it will have less/none of these current messy issue that many Flutter devs struggle with when it comes to theme colors for Flutter apps. Although I suspect many will think that M3 color design might be a bit complex. I think it seems pretty straight forward and makes a lot of sense. Looking forward to see how it gets implemented in Flutter. Might be a bit tricky with backward compatibility issues and potential challenges merging it with current widgets. Plus it will increase widget branching for color logic and calculation, potentially decreasing performance of current widgets. Hopefully not, will be interesting to see what the chosen approach is. Just new M3 widgets that uses new Theme designs and concepts, might be simplest, but not ideal either.

@talamaska
Copy link
Contributor

@rydmike From what I see, the scaffoldBackgroundColor is moved in a ScaffoldThemeData. Maybe that's a change after your recommendations. Anyways, thanks for keeping track of the ThemeData changes for all of us.

@rydmike
Copy link
Contributor

rydmike commented Nov 29, 2021

@talamaska Thanks, I noticed the addition as well. Perhaps it was a part of the plan all along. Probably so, since if it were not, it would result in new ThemeData that has no migration path for certain use cases.

@ghost
Copy link

ghost commented Feb 4, 2022

Currently, I am developing an app that should look Android'ish on Android and iOS'ish on iOS. So I use the MaterialApp for Android and the CupertinoApp for iOS. And I also try to get a Material- and CupertinoTheme together, both with dark and light colors. It isn't easy :(

I saw that CupertinoApp has only one theme parameter, and you use the CupertinoDynamicColor to define the light and dark colors (and some more).

Now I wonder why we don't have the same for the MaterialApp? Would it be a good idea to remove all these darkTheme, highContrastTheme, and highContrastDarkTheme from the MaterialApp and use a MaterialDynamicColor?

@rydmike
Copy link
Contributor

rydmike commented Feb 18, 2022

Additional colors to add to above list?

Should perhaps the following colors also be mentioned above among the ThemeData colors that should be deprecated/moved/removed?

  • primaryColorLight
  • primaryColorDark

They are currently not listed, but I doubt the intention is for them to remain. (cc @guidezpl)

primaryColorLight

A lighter color of primary. It is only used by CircleAvatar.
A potential replacement color is ColorScheme.primaryContainer.

primaryColorDark

A darker color of primary, it is used by CircleAvatar.
A potential replacement color is ColorScheme.onPrimaryContainer.

Challenges

The replacements would not be an exact match, so it would be breaking, but it matches the current design intent, if primaryContainer and onPrimaryContainer are using color tones as intended by M3 color design.

Status of primaryColorLight and pimaryColorDark

I wrote quite a bit about some past ThemeData color challenges here #65782. I was about to close the old issue, since most of the topics mentioned in it have been or are being addressed by the in progress ThemeData migration being tracked in this issue.

I did however not see anything addressing or mentioning primaryColorLight and primaryColorDark in ThemeData. Which does not mean it does not already exist, I might just have missed it.

The presence of primaryColorLight and primaryColorDark in ThemeData if kept, will continue to cause potential confusion.

Currently if you create a theme with ThemeData.from or just ThemeData and you give it a ColorScheme in colorScheme and don't provide a primarySwatch that would match in tones what you used for your primary in the ColorScheme, or directly give the color properties primaryColorLight and primaryColorDark values that fit your ColorScheme's primary color. Well, then you end up with primaryColorLight and primaryColorDark having default color values (blue hues, or grey in dark mode) that have nothing to do with whatever color you used in your ColorScheme as primary color, unless it happened to be a shade of blue close to the M2 blue swatch. Plus in the deprecate/move/remove list above, there is also this:

Deprecate primarySwatch from ThemeData factory constructor (8)

Which further complicates the continued existence of primaryColorLight and primaryColorDark in ThemeData.


EDIT Aug 3, 2022: Obviously primaryColor itself is also not mentioned in the ThemeData System Updates, should it also be included?

@guidezpl
Copy link
Member Author

Realized I never posted an update, but I unfortunately was (and remain) too busy with other work to land those ~10 closed PRs linked in the description. But they were all ready (minus conflicts), if someone wants to land them.

@TahaTesser
Copy link
Member

Realized I never posted an update, but I unfortunately was (and remain) too busy with other work to land those ~10 closed PRs linked in the description. But they were all ready (minus conflicts), if someone wants to land them.

I can do that. ✋

@guidezpl
Copy link
Member Author

guidezpl commented Feb 15, 2024

Thanks for offering! The only issue is, IIRC, that this batch more-or-less all had uses in the Google codebase that required removal or migration while landing the PR. In other words, these changes were not considered breaking per policy, but were in terms of the Google codebase.

auto-submit bot pushed a commit that referenced this issue May 16, 2024
Do a bit of sprucing up of `ThemeData` in anticipation of future work

Related: #91772
@Piinks Piinks added P2 Important issues not at the top of the work list c: tech-debt Technical debt, code quality, testing, etc. labels Jun 20, 2024
@QuncCccccc QuncCccccc mentioned this issue Jul 22, 2024
9 tasks
auto-submit bot pushed a commit that referenced this issue Jul 29, 2024
This PR is to make preparations to make `CardTheme` conform to Flutter's conventions for component themes:
* Added a `CardThemeData` class which defines overrides for the defaults for `Card` properties.
* Added 2 `CardTheme` constructor parameters: `CardThemeData? data` and `Widget? child`. This is now the preferred way to configure a `CardTheme`:
```dart
CardTheme(
  data: CardThemeData(color: xxx, elevation: xxx, ...),
  child: Card(...)
)
```

These two properties are made nullable to not break existing apps which has customized `ThemeData.cardTheme`.
* Changed the type of theme defaults from `CardTheme` to `CardThemeData`.

TODO: 
* Fix internal failures that may have breakages.
* Change the type of `ThemeData.cardTheme` from `CardTheme` to `CardThemeData`. This may cause breaking changes, a migration guide will be created.

Addresses the "theme normalization" sub project within #91772
TytaniumDev pushed a commit to TytaniumDev/flutter that referenced this issue Aug 7, 2024
This PR is to make preparations to make `CardTheme` conform to Flutter's conventions for component themes:
* Added a `CardThemeData` class which defines overrides for the defaults for `Card` properties.
* Added 2 `CardTheme` constructor parameters: `CardThemeData? data` and `Widget? child`. This is now the preferred way to configure a `CardTheme`:
```dart
CardTheme(
  data: CardThemeData(color: xxx, elevation: xxx, ...),
  child: Card(...)
)
```

These two properties are made nullable to not break existing apps which has customized `ThemeData.cardTheme`.
* Changed the type of theme defaults from `CardTheme` to `CardThemeData`.

TODO: 
* Fix internal failures that may have breakages.
* Change the type of `ThemeData.cardTheme` from `CardTheme` to `CardThemeData`. This may cause breaking changes, a migration guide will be created.

Addresses the "theme normalization" sub project within flutter#91772
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: tech-debt Technical debt, code quality, testing, etc. f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list team-design Owned by Design Languages team triaged-design Triaged by Design Languages team
Projects
No open projects
Status: 🚧 In Progress
Development

No branches or pull requests

10 participants