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

feat(form-field): add support for separate label and placeholder #8223

Merged
merged 2 commits into from
Nov 28, 2017

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Nov 3, 2017

  • Adds the ability for the user to specify both a label and placeholder text for a form field.
  • Renames all of the "floating placeholder" related terminology to refer to a "floating label" instead.
  • Aligns the input placeholder color with the Material spec.

Fixes #6194.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 3, 2017
@crisbeto
Copy link
Member Author

crisbeto commented Nov 3, 2017

@mmalerba I'm open to using the native label instead of mat-label, but it would require use to have a directive that selects all label nodes.

@jelbourn
Copy link
Member

jelbourn commented Nov 3, 2017

Could this be done without any breaking changes?

@crisbeto
Copy link
Member Author

crisbeto commented Nov 3, 2017

The only real breaking change is renaming the shouldPlaceholderFloat to shouldLabelFloat in the MatFormFieldControl interface @jelbourn, everything else is still re-exported under its old name. I can re-add the shouldPlaceholderFloat as an optional value and mark it as deprecated, but I suspect that not many people are implementing the interface.

@jelbourn
Copy link
Member

jelbourn commented Nov 3, 2017

Let's go with that- I want to avoid anything we have to call a breaking API change

@crisbeto
Copy link
Member Author

crisbeto commented Nov 4, 2017

Refactored to avoid breaking changes.

@mmalerba
Copy link
Contributor

mmalerba commented Nov 7, 2017

Still need to take a closer look at the code, but had a couple thoughts after looking at the demo:

  1. It should be possible to have in input with no label and a placeholder, but making that work now would be a breaking change. Maybe we can just file a new issue and update that when we're making breaking changes
  2. The placeholder needs an animation so it doesn't overlap with the label when the label is animating up. The spec has a video showing what it should look like

Thanks for doing this! Something I've been wanting to do for a while but haven't gotten the time

@crisbeto
Copy link
Member Author

crisbeto commented Nov 7, 2017

  1. Yeah I wanted to avoid breaking changes.
  2. We can't really animate the native placeholder attribute, the best we could do is delay showing it until the animation is done, but that means switching over the placeholder animations to Angular animations. We could do it in a follow-up.

@mmalerba
Copy link
Contributor

mmalerba commented Nov 7, 2017

we might not be able to do the sequenced animation, but could we at least slap a transition on the opacity? I think it will make it look a little nicer

@crisbeto
Copy link
Member Author

crisbeto commented Nov 7, 2017

We could do that, but it would be a progressive enhancement since not all browsers allow you to transition the native placeholder (e.g. Chrome does, but Firefox doesn't).

@crisbeto
Copy link
Member Author

Rebased and made it so the placeholder doesn't overlap the animating label @mmalerba, can you take another look?

@@ -8,24 +8,31 @@
<div class="mat-input-infix mat-form-field-infix">
<ng-content></ng-content>

<span class="mat-input-placeholder-wrapper mat-form-field-placeholder-wrapper">
<span class="mat-form-field-label-wrapper">
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure people are targeting those classes with custom styles, lets leave them for now with a TODO to remove them at the next major.

<!-- We add aria-owns as a workaround for an issue in JAWS & NVDA where the label isn't
read if it comes before the control in the DOM. -->
<label class="mat-input-placeholder mat-form-field-placeholder"
<label class="mat-form-field-label"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, leave the old classes too for now

}

_shouldLabelFloat() {
return this._canLabelFloat && (this._control.shouldLabelFloat ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic seems different than before, it didn't consider _canPlaceholderFloat before

Copy link
Member Author

Choose a reason for hiding this comment

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

If I remember correctly, some of the logic was done by combining CSS classes. I simplified it to make it easier to follow.

}

_hideControlPlaceholder() {
return !this._hasLabel() || !this._shouldLabelFloat();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the _canLabelFloat from above should be factored into this logic 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 _shouldLabelFloat call here already factors in _canLabelFloat.

@crisbeto crisbeto force-pushed the form-field-label branch 2 times, most recently from a348880 to dc4f44b Compare November 14, 2017 18:39
@crisbeto
Copy link
Member Author

I restored the classes and addressed the comments @mmalerba, can you take another look?

@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Nov 15, 2017
@jelbourn
Copy link
Member

@crisbeto ready to presubmit if you can rebase

@crisbeto
Copy link
Member Author

Rebased @jelbourn.

@tinayuangao
Copy link
Contributor

@crisbeto please rebase. Thanks!

* Adds the ability for the user to specify both a label and placeholder text for a form field.
* Renames all of the "floating placeholder" related terminology to refer to a "floating label" instead.
* Aligns the input placeholder color with the Material spec.

Fixes angular#6194.
@crisbeto
Copy link
Member Author

Rebased @tinayuangao.

@tinayuangao tinayuangao merged commit d6fec35 into angular:master Nov 28, 2017
tinayuangao pushed a commit to tinayuangao/material2 that referenced this pull request Nov 29, 2017
…ular#8223)

* feat(form-field): add support for separate label and placeholder

* Adds the ability for the user to specify both a label and placeholder text for a form field.
* Renames all of the "floating placeholder" related terminology to refer to a "floating label" instead.
* Aligns the input placeholder color with the Material spec.

Fixes angular#6194.

* fix: add transition to placeholder text
@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 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(input, select): separate the placeholder and label as shown in the official mocks
6 participants