Skip to content

fix(form-field): outline gap not calculated when appearance is provided through DI #12767

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

Jump to bottom
Merged
merged 1 commit into from
Aug 24, 2018
Merged

fix(form-field): outline gap not calculated when appearance is provided through DI #12767

merged 1 commit into from
Aug 24, 2018

Conversation

crisbeto
Copy link
Member

Fixes the outline gap not being calculated if the appearance is set through the default options, because it doesn't go through the setter and we're not guaranteed for the MutationObserver callback to fire at the right time.

Fixes #12765.

@crisbeto crisbeto added 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 labels Aug 21, 2018
@crisbeto crisbeto requested a review from mmalerba as a code owner August 21, 2018 07:59
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 21, 2018
@crisbeto
Copy link
Member Author

Marking as P2, because it appears to be a regression after 03527c6.

@crisbeto crisbeto force-pushed the 12765/form-field-outline-default branch from dcf010e to 8395c16 Compare August 21, 2018 08:52
@mmalerba mmalerba added the regression This issue is related to a regression label Aug 21, 2018
@OlufemiOyewole OlufemiOyewole mentioned this pull request Aug 21, 2018
Closed
josephperrott
josephperrott approved these changes Aug 22, 2018
View reviewed changes
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

@ngbot
Copy link

ngbot bot commented Aug 22, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@crisbeto crisbeto force-pushed the 12765/form-field-outline-default branch from 8395c16 to c9f9e73 Compare August 22, 2018 20:53
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Aug 22, 2018
mmalerba
mmalerba reviewed Aug 22, 2018
View reviewed changes
src/lib/form-field/form-field.ts Outdated
set appearance(value: MatFormFieldAppearance) {
const oldValue = this._appearance;
this._appearance = value;

if (value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think previously if you passed null it was the same as setting it to legacy. This seems to be changing it to ignore null, is there a reason for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can do this._appearance = this._defaults && this._defaults.appearance || 'legacy' instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for adding the null check was to avoid having to repeat this._appearance = this._defaults && this._defaults.appearance || 'legacy' in two places.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah but people can still set null via a template binding

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@mmalerba mmalerba removed the action: merge The PR is ready for merge by the caretaker label Aug 22, 2018
@crisbeto
fix(form-field): outline gap not calculated when appearance is provid…
6775ea6 
…ed through DI

Fixes the outline gap not being calculated if the `appearance` is set through the default options, because it doesn't go through the setter and we're not guaranteed for the `MutationObserver` callback to fire at the right time.

Fixes angular#12765.
@crisbeto crisbeto force-pushed the 12765/form-field-outline-default branch from c9f9e73 to 6775ea6 Compare August 23, 2018 18:04
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Aug 23, 2018
@jelbourn jelbourn merged commit 8e49388 into angular:master Aug 24, 2018
jelbourn pushed a commit that referenced this pull request Aug 29, 2018
@crisbeto @jelbourn
fix(form-field): outline gap not calculated when appearance is provid…
8b9d283 
…ed through DI (#12767)

Fixes the outline gap not being calculated if the `appearance` is set through the default options, because it doesn't go through the setter and we're not guaranteed for the `MutationObserver` callback to fire at the right time.

Fixes #12765.
@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 Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mmalerba mmalerba mmalerba left review comments

@josephperrott josephperrott josephperrott approved these changes

Assignees

@josephperrott josephperrott

@mmalerba mmalerba

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround regression This issue is related to a regression target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Outline gap not calculated with MAT_FORM_FIELD_DEFAULT_OPTIONS in 6.4.6
5 participants
@crisbeto @josephperrott @mmalerba @jelbourn @googlebot