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

fix(material/toolbar): Update icon button color to match label text color token #30280

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amysorto
Copy link
Contributor

@amysorto amysorto commented Jan 7, 2025

Fixes #30279

Note: this only affects icon buttons that have the mat-unthemed class.

image

@amysorto amysorto requested a review from a team as a code owner January 7, 2025 17:03
@amysorto amysorto requested review from crisbeto and mmalerba and removed request for a team January 7, 2025 17:03
@@ -56,6 +56,8 @@ $height-mobile-portrait: 56px !default;
@include token-utils.use-tokens(
tokens-mat-toolbar.$prefix, tokens-mat-toolbar.get-token-slots()) {
$color-token: token-utils.get-token-variable(container-text-color);
// Update icon button color to match label text color for consistency while overriding.
--mdc-icon-button-icon-color: $color-token;
Copy link
Member

Choose a reason for hiding this comment

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

We should do this through use-tokens, instead of hardcoding the name. See in the next few lines how it's done for the outlined and text buttons.

@amysorto amysorto force-pushed the toolbar-icon-color-overrides branch from c0be42b to 1d35c12 Compare January 7, 2025 18:04
@richardsengers
Copy link

@amysorto what is the status of this PR? it's not yet merged in the latest release :-(

@richardsengers
Copy link

@crisbeto do you know why this PR is still open?It is a bit of a showstopper

@mmalerba mmalerba added the target: patch This PR is targeted for the next patch release label Feb 19, 2025
@csvn
Copy link
Contributor

csvn commented Mar 26, 2025

I just encountered the issue this PR fixes for toolbar icons when trying to adapt the M3 theme. Is this PR being blocked by something? I can't see this fix included in any existing versions.

image

@mmalerba mmalerba force-pushed the toolbar-icon-color-overrides branch from 1d35c12 to bf9e795 Compare April 4, 2025 15:53
@angular-robot angular-robot bot requested a review from crisbeto April 4, 2025 15:53
@mmalerba mmalerba force-pushed the toolbar-icon-color-overrides branch from bf9e795 to 7f28810 Compare April 4, 2025 15:54
@mmalerba mmalerba self-assigned this Apr 4, 2025
@mmalerba mmalerba added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged and removed target: patch This PR is targeted for the next patch release labels Apr 4, 2025
@mmalerba
Copy link
Contributor

mmalerba commented Apr 5, 2025

There are hundreds of screenshots in g3 that wind up looking worse with this change. It seems to cause the exact issue its attempting to fix in some apps, likely because they're already working around it somehow. It'll take some work to land

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker area: material/toolbar presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(Toolbar): toolbar-overrides container-text-color does not set var(--mat-sys-on-primary) to icons
5 participants