Skip to content

docs(material/theming): Add docs for the new base theming dimension #27992

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
Oct 26, 2023

Conversation

mmalerba
Copy link
Contributor

@mmalerba mmalerba commented Oct 24, 2023

@mmalerba mmalerba added docs This issue is related to documentation target: rc This PR is targeted for the next release-candidate labels Oct 24, 2023
@mmalerba mmalerba added this to the 17.0.0 milestone Oct 24, 2023
@mmalerba mmalerba requested a review from andrewseguin October 24, 2023 19:10
@mmalerba mmalerba requested a review from jelbourn as a code owner October 24, 2023 19:10
@mmalerba mmalerba changed the title docs(material/theming): Add docs the new base theming dimension docs(material/theming): Add docs for the new base theming dimension Oct 24, 2023
@angular-robot angular-robot bot added the area: docs Related to the documentation label Oct 24, 2023
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor suggestions

@@ -235,6 +238,95 @@ your project's `angular.json` file][adding-styles].

[adding-styles]: https://angular.io/guide/workspace-config#styles-and-scripts-configuration

#### Theming dimensions

Angular Material themes are divided along several dimensions:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Angular Material themes are divided along several dimensions:
Angular Material themes are organized into four categories:

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 was hoping to use the term "dimension" so we have a way to talk about these categories in a way that feels slightly less general than "category"

Comment on lines 246 to 250
: Common base styles for the design system. These styles don't change based on your configured
colors, typography, or density, so they only need to be included once per application. These
styles may include things such as border-radius, border-width, etc. All components have a base
mixin that can be used to include its base styles. (For example,
Copy link
Member

Choose a reason for hiding this comment

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

Should we say something about only most apps needing to import them once, but that you can still include it multiple times if you want to change the base styles for different parts of your app?

Copy link
Contributor Author

@mmalerba mmalerba Oct 24, 2023

Choose a reason for hiding this comment

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

Well the thing is right now you can't really change it. The base styles are basically values for all of the tokens that don't depend on color, typography, or density, but may vary between M2, M3, or some future Material Design version. Since currently M2 is the only one available, the mixin always emits the same values.

In the future there could maybe be a use case for it, if some app wanted to display M2 vs M3 conditionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another future use case is maybe once we have a Sass API to allow per-token customizations, but again thats not something we have now, so for now it just feels simplest to say "include it once"

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Oct 26, 2023
@mmalerba mmalerba merged commit e2ac195 into angular:main Oct 26, 2023
mmalerba added a commit that referenced this pull request Oct 26, 2023
@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 Nov 26, 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 area: docs Related to the documentation docs This issue is related to documentation target: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants