Skip to content

refactor(material-experimental/theming): condense matx.theme and matx.retheme into a single mixin #27309

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

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

mmalerba
Copy link
Contributor

Rather than having a separate mixin for "retheming" we have a single mixin matx.theme that can either emit fallback token defaults or not depending on how its configured. Added matx.token-defaults which can be used to configure the fallbacks.

@mmalerba mmalerba added the target: patch This PR is targeted for the next patch release label Jun 15, 2023
@mmalerba mmalerba requested a review from devversion as a code owner June 15, 2023 00:34
….retheme into a single mixin

Rather than having a separate mixin for "retheming" we have a single
mixin `matx.theme` that can either emit fallback token defaults or not
depending on how its configured. Added `matx.token-defaults` which can
be used to configure the fallbacks.
@devversion devversion removed their request for review June 16, 2023 14:08
matx.card(),
matx.checkbox(),
)
matx.token-defaults(mat.m2-tokens-from-theme($theme)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewseguin I discovered that you left a comment on an isolated commit that is somehow no longer part of this actual PR, so transferring the comment here:

I'm all in favor of removing retheme and making a single API that users need to call for applying their styles.

However, there's a lot of layers here that I don't expect users to understand, and taking a step back it seems a little odd to read. We created a theme with define-light-theme but we don't pass that to a theme mixin, but instead we pass that to a "get-tokens" that gets passed to "defaults" and finally put into "theme.

It seems like the theme mixin should be getting some kind of first-class theme object here. I think the confusion comes from trying to support our old theme object. E.g. why does our theme have a mixin called define-light-theme when light has nothing to do with density or typography?

Is there a way to at least eliminate the need for calling define-light-theme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for creating a theme and converting it to tokens is because this example shows how an M2 app would use the new API. The only way we really have to get M2 tokens is to generate them from the legacy theme objects. We had to make up our own tokens for MDC, based on the theme since MDC doesn't give us M2 tokens.

For an M3 app, they would supply the tokens directly without the need for creating a theme first. We still need to figure out exactly how users are expected to get those:

  • export from MDC's theme builder (we still need to figure out how that works)
  • generate from a single color swatch (I think we've been told this can't be done in Sass for now)
  • some other way ??

To make the M2 API less clunky, we could make a single function that takes the theme, generates tokens from it, and then returns a configuration for the new theme system as a one-shot:

@include matx.theme(
  matx.token-defaults-from-legacy-theme($app-theme),
  matx.checkbox(),
  ...
);

It's not going to remove the need to call define-light-theme to generate $app-theme though.

@mmalerba mmalerba added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release target: minor This PR is targeted for the next minor release labels Jul 7, 2023
@mmalerba mmalerba merged commit 5f5c516 into angular:main Jul 7, 2023
stephenrca pushed a commit to stephenrca/components that referenced this pull request Aug 2, 2023
….retheme into a single mixin (angular#27309)

Rather than having a separate mixin for "retheming" we have a single
mixin `matx.theme` that can either emit fallback token defaults or not
depending on how its configured. Added `matx.token-defaults` which can
be used to configure the fallbacks.
@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 Aug 7, 2023
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 target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants