Skip to content

docs(examples): all examples should use mat-form-field's fill appearance by default #18252

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
Jan 26, 2020
Merged

docs(examples): all examples should use mat-form-field's fill appearance by default #18252

merged 1 commit into from
Jan 26, 2020

Conversation

Splaktar
Copy link
Contributor

@Splaktar Splaktar commented Jan 22, 2020 •
edited
Loading

  • stop using the soon to be deprecated legacy appearance by default
  • fix issues where labels were specified in the placeholder attribute
    • instead of a mat-label

Relates to #14792

@Splaktar Splaktar added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent docs This issue is related to documentation pr: merge safe target: patch This PR is targeted for the next patch release labels Jan 22, 2020
@Splaktar Splaktar requested review from jelbourn and mmalerba January 22, 2020 13:30
@Splaktar Splaktar requested a review from a team as a code owner January 22, 2020 13:30
@Splaktar Splaktar self-assigned this Jan 22, 2020
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 22, 2020
@Splaktar Splaktar mentioned this pull request Jan 22, 2020
Closed
crisbeto
crisbeto reviewed Jan 22, 2020
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The changes seem fine, but the PR title is talking about the appearance being switched to fill, but that's not what's happening. Instead, all the placeholders are being replaced with labels.

devversion
devversion reviewed Jan 22, 2020
View reviewed changes
@Splaktar
Copy link
Contributor Author

all the placeholders are being replaced with labels.

That's due to the way placeholders work being different between the legacy and all other appearances. Legacy floats the placeholder like a label whereas the others do not. This meant that with the fill appearance, the "label" disappeared once a value was entered. With these changes, the label text floats properly when a value exists.

With the previous legacy style, label content was placed in placeholders. With the new appearances, label content goes in mat-labels and placeholder content in the placeholder attribute.

@crisbeto
Copy link
Member

I know the context behind the change and it's fine, but it doesn't align with the commit message.

@Splaktar Splaktar force-pushed the form-field-fill-default-appearance branch from ce208a7 to 7105883 Compare January 22, 2020 21:51
@Splaktar
Copy link
Contributor Author

it doesn't align with the commit message

Added details about placeholders and labels. Please let me know if that isn't sufficient.

devversion
devversion approved these changes Jan 23, 2020
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM. I'm still unsure if it could cause confusion for the code previews in the docs. It shouldn't be a big deal though.

@Splaktar
docs(examples): all examples should use mat-form-field's fill appeara…
49857a3 
…nce by default

- stop using the soon to be deprecated legacy appearance by default
- fix issues where labels were specified in the `placeholder` attribute
  - instead of a `mat-label`

Relates to angular#14792
@Splaktar Splaktar force-pushed the form-field-fill-default-appearance branch from 7105883 to 49857a3 Compare January 23, 2020 12:40
jelbourn
jelbourn approved these changes Jan 26, 2020
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jan 26, 2020
@jelbourn jelbourn merged commit 0e30214 into angular:master Jan 26, 2020
jelbourn pushed a commit that referenced this pull request Jan 26, 2020
@Splaktar @jelbourn
docs(examples): all examples should use mat-form-field's fill appeara…
cbbccaf 
…nce by default (#18252)

- stop using the soon to be deprecated legacy appearance by default
- fix issues where labels were specified in the `placeholder` attribute
  - instead of a `mat-label`

Relates to #14792

(cherry picked from commit 0e30214)
@Splaktar Splaktar deleted the form-field-fill-default-appearance branch January 26, 2020 22:43
yifange pushed a commit to yifange/components that referenced this pull request Jan 30, 2020
@Splaktar @yifange
docs(examples): all examples should use mat-form-field's fill appeara…
f34d269 
…nce by default (angular#18252)

- stop using the soon to be deprecated legacy appearance by default
- fix issues where labels were specified in the `placeholder` attribute
  - instead of a `mat-label`

Relates to angular#14792
@Splaktar Splaktar mentioned this pull request Feb 6, 2020
Merged
@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 Mar 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto left review comments

@devversion devversion devversion approved these changes

@jelbourn jelbourn jelbourn approved these changes

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees

@Splaktar Splaktar

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement docs This issue is related to documentation P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent 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.
5 participants
@Splaktar @crisbeto @jelbourn @devversion @googlebot