Skip to content

fix(material-experimental/mdc-menu): fix bold font when using 2014 typography #23614

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix(material-experimental/mdc-menu): fix bold font when using 2014 typography #23614

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/material-experimental/mdc-menu/_menu-theme.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
@use 'sass:map';
@use '@material/theme/theme-color' as mdc-theme-color;
@use '@material/theme/theme' as mdc-theme;
@use '@material/menu-surface' as mdc-menu-surface;
Expand Down Expand Up @@ -47,6 +48,18 @@
@mixin typography($config-or-theme) {
$config: typography.private-typography-to-2018-config(
theming.get-typography-config($config-or-theme));

// When the 2014 typography config is mapped to the 2018 config, the font-weight that winds up
// getting used for menu items looks slightly off. In this case we override it to a value that
// looks better.
@if (typography.private-typography-is-2014-config($config-or-theme)) {
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 fix this at a higher level? I think that select and autocomplete (and maybe list?) use the same set of mixins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a good point. Let's merge this PR because its smaller in scope and will unblock me for the menu migrations I'm working on, but as a follow-up I'll try to address this more holistically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So @andrewseguin made the opposite hack for table when he was working on that migration. There the font wasn't bold enough. Looking into it more I found that if we just ignore how the levels are named, they actually map better visually if we just swap them, so that's what I've done in #23618 which allows us to remove both this hack and Andrew's 🎉

$config: map.merge($config, (
body-1: map.merge(map.get($config, body-1), (
font-weight: 400
))
));
}

@include mdc-helpers.mat-using-mdc-typography($config) {
@include mdc-menu-surface.core-styles(mdc-helpers.$mat-typography-styles-query);

Expand Down