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

feat(icon): add --md-icon-size token #5150

Merged
merged 5 commits into from
Nov 14, 2023

Conversation

scherler
Copy link
Contributor

@scherler scherler commented Nov 2, 2023

Fixes #5055

In the old version, the size was controlled by a css variable font-size: var(--mdc-icon-size, 24px); Our development is heavily dependent on this to control the size, and the quickest update route is to get back the md-icon-size token.

Addresses #5055 see as well #5055 (comment)

Signed-off-by: Thorsten Scherler <[email protected]>
Copy link

google-cla bot commented Nov 2, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

// go/keep-sorted end
);

@function _get-new-tokens($exclude-hardcoded-values) {
@return (
// go/keep-sorted start
font: if($exclude-hardcoded-values, null, 'Material Symbols Outlined'),
size: if($exclude-hardcoded-values, null, '24px'),
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 am not sure about whether this is the correct way to set the default

Copy link
Contributor Author

@scherler scherler Nov 3, 2023

Choose a reason for hiding this comment

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

I think it is correct, however, the default font is actually not injected font-family: var(--md-icon-font,); which you can see in current main branch

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks correct, but you'll want to remove the string quotes around the pixel value

size: if($exclude-hardcoded-values, null, 24px),

icon/internal/_icon.scss Outdated Show resolved Hide resolved
@asyncLiz asyncLiz self-assigned this Nov 2, 2023
@asyncLiz asyncLiz self-requested a review November 2, 2023 21:32
Signed-off-by: Thorsten Scherler <[email protected]>
@scherler scherler changed the title [icon_size] add size to tokens feat(icon): add font_size to tokens so the size in controlled by a css variable Nov 3, 2023
@scherler scherler changed the title feat(icon): add font_size to tokens so the size in controlled by a css variable feat(icon): add font_size to controll size by a css variable Nov 3, 2023
@asyncLiz
Copy link
Collaborator

asyncLiz commented Nov 6, 2023

Thanks for the PR! We'll take a look at it during our weekly sync

@scherler
Copy link
Contributor Author

scherler commented Nov 8, 2023

@asyncLiz when is your weekly? I have a bet with my manager that this one gets merged quicker than a company one. 😉 😁 BTW my team does https://www.cloudbees.com/capabilities/continuous-integration/pipeline-explorer where we use https://github.com/scherler/sirocco-wc/ and mwc as base design lib.

@asyncLiz
Copy link
Collaborator

asyncLiz commented Nov 9, 2023

We agree with the addition, but I might not get to reviewing it until next week. Thanks for your patience!

Copy link
Collaborator

@asyncLiz asyncLiz left a comment

Choose a reason for hiding this comment

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

Thanks and sorry for the delay! Left a few comments to address :)

icon/internal/_icon.scss Show resolved Hide resolved
color: inherit;
font-variation-settings: inherit;
font-weight: 400;
font-family: var(--md-icon-font, #{map.get($tokens, font)});
font-family: map.get($tokens, font);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add single quotes around the keys in map.get?

map.get($tokens, 'font');

// go/keep-sorted end
);

@function _get-new-tokens($exclude-hardcoded-values) {
@return (
// go/keep-sorted start
font: if($exclude-hardcoded-values, null, 'Material Symbols Outlined'),
size: if($exclude-hardcoded-values, null, '24px'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks correct, but you'll want to remove the string quotes around the pixel value

size: if($exclude-hardcoded-values, null, 24px),

@asyncLiz asyncLiz changed the title feat(icon): add font_size to controll size by a css variable feat(icon): add --md-icon-size token Nov 13, 2023
@scherler scherler requested a review from asyncLiz November 14, 2023 12:29
Copy link
Collaborator

@asyncLiz asyncLiz left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@copybara-service copybara-service bot merged commit 128abcb into material-components:main Nov 14, 2023
3 checks passed
@scherler scherler deleted the icon_size branch November 15, 2023 06:14
@scherler
Copy link
Contributor Author

@asyncLiz thanks to you

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.

[Feature Request] A simple way to change <md-icon> size
2 participants