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.

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

Conversation

mmalerba
Copy link
Contributor

No description provided.

@mmalerba mmalerba added P2 The issue is important to a large percentage of users, with a workaround G This is is related to a Google internal issue target: patch This PR is targeted for the next patch release labels Sep 21, 2021
@mmalerba mmalerba requested a review from crisbeto as a code owner September 21, 2021 22:19
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 21, 2021
Copy link
Contributor

@wagnermaciel wagnermaciel left a comment

Choose a reason for hiding this comment

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

lgtm

@wagnermaciel wagnermaciel added the action: merge The PR is ready for merge by the caretaker label Sep 22, 2021
// 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 🎉

@mmalerba mmalerba added blocked This issue is blocked by some external factor, such as a prerequisite PR and removed action: merge The PR is ready for merge by the caretaker labels Sep 23, 2021
@mmalerba
Copy link
Contributor Author

Closing in favor of #23618

@mmalerba mmalerba closed this Sep 24, 2021
@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 Oct 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocked This issue is blocked by some external factor, such as a prerequisite PR cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants