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

fix(material-experimental/mdc-form-field): add workaround for incorrect MDC label size #23137

Closed
wants to merge 1 commit into from

Conversation

mmalerba
Copy link
Contributor

@mmalerba mmalerba commented Jul 8, 2021

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 Jul 8, 2021
@mmalerba mmalerba requested a review from devversion as a code owner July 8, 2021 23:53
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 8, 2021
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Jul 9, 2021
// We have to target the inner <mat-label> rather than the <label> because MDC applies
// high-specificity rules that incorrectly hard-code the size as 1rem on the label.
.mat-mdc-form-field mat-label,
.mat-mdc-form-field mat-label::after,
Copy link
Member

@devversion devversion Jul 9, 2021

Choose a reason for hiding this comment

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

Should this be .mdc-floating-label::after? The asterisk is a pseudo-element of the parent element, not mat-label, or is that targeting something else?

@devversion
Copy link
Member

devversion commented Jul 9, 2021

@mmalerba This would be fixed by #23005 too. Your approach seems easier to maintain, but unfortunately breaks in pre-rendering where the non-upgraded notched-outline has some special styling to make the label appear as it would in the hydrated component. e.g. this is how it looks now pre-rendered:

pre-rendered-miles

With #23005 it will pre-rendered correctly too (although it being a bit more code to maintain; but worth it IMO; especially since MDC puts effort into pre-rendering visuals)

pre-rendered-my-pr

(I'm just removing merge ready; so that we can chat about it)

@devversion devversion removed the action: merge The PR is ready for merge by the caretaker label Jul 9, 2021
@mmalerba
Copy link
Contributor Author

@devversion I do think we should make your change, though I'm not sure if it will be sufficient to overpower the specificity of some of the styles MDC sets. I'll put mine on pause until yours goes in, then we can verify and see if any additional changes need to happen to workaround.

@mmalerba mmalerba added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Jul 12, 2021
@devversion
Copy link
Member

@mmalerba Sounds good 👍 I've rebased the other PR; should be good to go.

@mmalerba
Copy link
Contributor Author

Closing since #23005 solved this issue

@mmalerba mmalerba closed this Sep 22, 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 23, 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.

None yet

3 participants