Skip to content

Conversation

@andrewseguin
Copy link
Contributor

No description provided.

@andrewseguin andrewseguin requested a review from a team as a code owner June 10, 2024 20:43
@andrewseguin andrewseguin requested review from amysorto and crisbeto and removed request for a team June 10, 2024 20:43
@angular-robot angular-robot bot added the area: docs Related to the documentation label Jun 10, 2024
"file":"toolbar-multirow-example.css",
"region":"toolbar-position-content-style"}) -->

### Theming
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this doc comment be updated as well since that generates what is on the API tab? https://github.com/angular/components/blob/main/src/material/toolbar/toolbar.ts#L49.

Maybe something like: /** Palette color of the toolbar. Not recommended in M3, for more information see https://material.angular.io/guide/material-2-theming#optional-add-backwards-compatibility-styles-for-color-variants. */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes definitely - I'm thinking we'll have one single PR that goes around to update all color inputs with some consistent message

Choose a reason for hiding this comment

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

If color is not supported in MD3 or not recommended, then why keep using it in the angular material page? Its misguiding. (Also, I personally am against toolbar not having a pallete color, I find it hostile to how a business wants to present its brand.)

Choose a reason for hiding this comment

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

in #28805 (comment) I made a comment of how to adapt theme colors to the toolbar.

Option 1:

@mixin app-theme($theme, $type: "light") {
  @if $type == "light" {
    @include mat.all-component-themes($theme);
    .mat-toolbar {
      --mat-toolbar-container-text-color: #{mat.get-theme-color(
          $theme,
          primary,
          100
        )};
      --mat-toolbar-container-background-color: #{mat.get-theme-color(
          $theme,
          primary
        )};
      --mdc-icon-button-icon-color: #{mat.get-theme-color(
          $theme,
          primary,
          100
        )};
    }
 } @else {
    @include mat.all-component-colors($theme);
    .mat-toolbar {
      --mat-toolbar-container-text-color: #{mat.get-theme-color(
          $theme,
          primary
        )};
      --mat-toolbar-container-background-color: #{mat.get-theme-color(
          $theme,
          primary
        )};
      --mdc-icon-button-icon-color: #{mat.get-theme-color($theme, primary, 20)};
    }
  }
}

html {
  // .app-light-theme (default)
  @include app-theme($app-light-theme, "light");

  .app-dark-theme {
    @include app-theme($app-dark-theme, "dark");
  }

  @media (prefers-color-scheme: light) {
    .app-auto-theme {
      @include app-theme($app-light-theme, "light");
    }
  }

  @media (prefers-color-scheme: dark) {
    .app-auto-theme {
      @include app-theme($app-dark-theme, "dark");
    }
  }
}

Option 2:

@mixin app-theme($theme, $type: "light") {
  @if $type == "light" {
    @include mat.all-component-themes($theme);
    .mat-toolbar {
      background: mat.get-theme-color($theme, primary);
      .mat-mdc-icon-button {
        color: mat.get-theme-color($theme, primary, 100);
      }
    }
 } @else {
    @include mat.all-component-colors($theme);
    .mat-toolbar {
      background: mat.get-theme-color($theme, primary);
      .mat-mdc-icon-button {
        color: mat.get-theme-color($theme, primary, 20);
      }
    }
  }
}

html {
  // .app-light-theme (default)
  @include app-theme($app-light-theme, "light");

  .app-dark-theme {
    @include app-theme($app-dark-theme, "dark");
  }

  @media (prefers-color-scheme: light) {
    .app-auto-theme {
      @include app-theme($app-light-theme, "light");
    }
  }

  @media (prefers-color-scheme: dark) {
    .app-auto-theme {
      @include app-theme($app-dark-theme, "dark");
    }
  }
}

Any Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I empathize with wanting color in the toolbar - we also think it looks better for branding. However, we're beholden to the spec and designers here, and our goal is to match their direction

Choose a reason for hiding this comment

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

Thanks for the reply. I understand, is the Angular Material page be updated to adhere to the specs? As it is it, I believe it contributes to the confusion.

@andrewseguin andrewseguin added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jun 17, 2024
@andrewseguin andrewseguin merged commit b4d01dc into angular:main Jun 17, 2024
andrewseguin added a commit that referenced this pull request Jun 17, 2024
DBowen33 pushed a commit to DBowen33/components that referenced this pull request Jul 3, 2024
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: docs Related to the documentation target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants