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

DevtoolsToggleButton and ServiceExtensionButton have same text color #8377

Merged
merged 9 commits into from
Oct 2, 2024

Conversation

elliette
Copy link
Member

Fixes #8365

Unselected:

Screenshot 2024-09-30 at 10 52 41 AM

Selected:

Screenshot 2024-09-30 at 10 52 50 AM

Before this change:

image

@elliette elliette requested review from bkonyi and a team as code owners September 30, 2024 18:38
label: label,
minScreenWidthForTextBeforeScaling:
minScreenWidthForTextBeforeScaling,
child: ImageIconLabel(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the correct fix here? or is the fix to apply the color properly in material icon label? devtools_app_shared is a public package so we should be careful about making changes here that may potentially break downstream UIs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated MaterialIconLabel instead. This will however change the appearance of the label for any clients who didn't specify a color. Previously if color was null, the Flutter framework would decide on the color for the icon based on the iconTheme, but the text label color wouldn't match because the color there is null.

I think from the code it was intended that the icon and its label would always have the same color. If we want to only fix the DevtoolsToggleButton case I could explicitly pass in the color parameter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we fix this without making changes to devtools_app_shared?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - I added some optional parameters, but this shouldn't affect any existing widgets from devtools_app_shared

@elliette elliette merged commit 62a7877 into flutter:master Oct 2, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DevToolsToggle button and ServiceExtensionButton have different font colors
3 participants