-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
Update Dark Theme disabledColor to White38 #35136
Conversation
I'll hold off on sending the breaking change email until I've gotten some PR feedback on this just to make sure that everything seems ok |
@@ -470,7 +470,7 @@ class ButtonThemeData extends Diagnosticable { | |||
|
|||
Color _getDisabledColor(MaterialButton button) { | |||
return getBrightness(button) == Brightness.dark | |||
? colorScheme.onSurface.withOpacity(0.30) // default == Colors.white30 | |||
? colorScheme.onSurface.withOpacity(0.38) // default == Colors.white38 |
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.
Both conditions in are now the same here, so the conditional is unnecessary
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.
Made the fix, as well as updated the API docs since I missed updating those as well
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.
LGTM
@@ -489,7 +483,7 @@ class ButtonThemeData extends Diagnosticable { | |||
return button.textColor; | |||
if (button.disabledTextColor != null) | |||
return button.disabledTextColor; | |||
return _getDisabledColor(button); | |||
return colorScheme.onSurface.withOpacity(0.38); |
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.
NICE
* Add Colors.white38 * Update ThemeData.disabledColor and ButtonThemeData.disabledColor to Colors.white38 * Update pre-existing tests to expect Colors.white38 instead of Colors.white30 * Update API documentation to reflect these changes
Description
Currently, Dark Theme in Material sets disabled states to Colors.white30. However, according to the Material Design specs for dark theme, it should be White 38% instead. This PR reflects this change.
Related Issues
Addresses #33149
Tests
I added the following tests:
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?
Yes, this is a breaking change (Please read Handling breaking changes).
No, this is not a breaking change.