[Icons] Updating default icon theme values.#20840
[Icons] Updating default icon theme values.#20840willlarche merged 10 commits intoflutter:masterfrom
Conversation
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
- This PR should also update the default disabled color for buttons (including IconButton). Currently it's initialized like this:
disabledColor ??= isDark ? Colors.white30 : Colors.black26;
I don't believe that's correct.
- Before committing this change we need to make sure that the resulting contrast is sufficient for a11y.
- This PR will need a tests.
There was a problem hiding this comment.
I'm afraid that is probably overachieving. I don't think we want to start applying 0.87 to the opacity of all icons that specify a color, even though doing so is probably what's intended by the latest spec (https://material.io/design/iconography/system-icons.html#color).
There's a fine backwards compatibility line here. The appearance of an app that has specified an icon color explicitly shouldn't change; an app that's picking up the default icon color can change.
|
It looks like MaterialButton needs a similar change: Should just be LIkewise for: And maybe for: |
|
What about |
|
The button changes look good. This issue's description should include before and after screenshots, to make it easier for developers to see what's changing. |
There was a problem hiding this comment.
Wouldn't tester.widget<Text>(find.text(testText)).style work here? The test probably doesn't need to depend on the fact that the Text widget is defined in terms of RichText.
There was a problem hiding this comment.
No. You have to look for the RichText widget. The style on Text widgets is null when there's no theme. But it does draw. Just with private API to the RichText.
willlarche
force-pushed
the
fix-3188-icon-defaultColors
branch
from
2433b74 to
0e39147
Compare
3 participants
Closes #3188