refactor(input): rename `input-container` to `form-field`... #6331

mmalerba · 2017-08-07T20:27:04Z

and move all communication with mdInput into a well defined interface. This is a pre-requisite to making md-select share a container element with mdInput.

This is a first-pass PR that tries to avoid breaking changes as much as possible. We can update Google's codebase and remove the old selectors and class names in the future.

@tinayuangao This should also allow you to integrate md-chip-list with the input more easily.

Incremental step toward #2124

BREAKING CHANGE: MdInputContainer is now MdFormField and MdInputDirective is now MdInput

separate interface

mmalerba · 2017-08-07T21:48:06Z

here's a demo with the changes: https://mmalerba-demo2.firebaseapp.com/, basically nothing should change from a user's perspective

jelbourn · 2017-08-08T00:21:56Z

src/lib/autocomplete/autocomplete-trigger.ts

@@ -410,8 +411,9 @@ export class MdAutocompleteTrigger implements ControlValueAccessor, OnDestroy {

    // If it's used in a Material container, we should set it through
    // the property so it can go through the change detection.
-    if (this._inputContainer) {
-      this._inputContainer._mdInputChild.value = inputValue;
+    if (this._inputContainer &&


Rename _inputContainer to _formField?

jelbourn · 2017-08-08T00:23:41Z

src/lib/autocomplete/autocomplete-trigger.ts

-    if (this._inputContainer) {
-      this._inputContainer._mdInputChild.value = inputValue;
+    if (this._inputContainer &&
+        this._inputContainer._control instanceof MdInput) {


Why does this need to check MdInput specifically now? Add a comment?

Hmm now that I think about it, its probably better to make the value property part of the MdFormFieldControl interface so this doesn't have to be hardcoded for MdInput. I was only thinking aboutmdInput and md-select when I wrote this, but its reasonable to want autocomplete to work with a user's custom input control as well

jelbourn · 2017-08-08T00:31:43Z

src/lib/form-field/form-field.ts

+  host: {
+    // Remove align attribute to prevent it from interfering with layout.
+    '[attr.align]': 'null',
+    'class': 'mat-input-container mat-form-field',


Add a comment that mat-input-container is deprecated and will be removed?

jelbourn · 2017-08-08T00:33:14Z

src/lib/form-field/form-field.ts

+  // MdInput is a directive and can't have styles, so we need to include its styles here.
+  // The MdInput styles are fairly minimal so it shouldn't be a big deal for people who aren't using
+  // MdInput.
+  styleUrls: ['form-field.css', '../input/input.css'],


This may be a problem in Google-land (but maybe not; not totally sure)

If it winds up being a problem I can move the file under form-field/ with a note about why its there

jelbourn · 2017-08-08T00:34:39Z

src/lib/form-field/form-field.ts

+  ],
+  host: {
+    // Remove align attribute to prevent it from interfering with layout.
+    '[attr.align]': 'null',


Do we still use align on this?

No, but we do on md-hint, so moved this to there

jelbourn · 2017-08-08T00:38:08Z

src/lib/form-field/form-field.ts

+  changeDetection: ChangeDetectionStrategy.OnPush,
+})
+
+export class MdFormField implements AfterViewInit, AfterContentInit, AfterContentChecked {


In public_api.ts, we should re-export this as MdInputContainer to stay backwards compatible with anyone importing it (about 18 instances within Google)

jelbourn · 2017-08-08T00:42:19Z

src/lib/input/index.ts

-    MdSuffix,
+    // TODO(mmalerba): We import and re-export the form field module since all existing users of
+    // `MdInput` will need this to continue using `md-input-container`. We may want to keep this
+    // long term since the `MdInput` directive will almost always be used with `md-form-field`.


Yeah, long-term I think the fact that form-field is its own thing is an implementation detail (similar to mdLine) that the users don't need to care about

Ok so then input, select, and any other components that wind up using it should all re-export it in their respective modules?

Updated the comment to indicate that we expect to keep this long term

jelbourn · 2017-08-08T00:46:54Z

src/lib/form-field/form-field.ts

+
+
+/** An interface which allows a control to work inside of a `MdFormField`. */
+export abstract class MdFormFieldControl {


Make this an interface since it doesn't have any default implementations? I recall Misko mentioning that abstract classes should be avoided for payload size reasons

I did this because I need to use the class itself as the injection token so it can be used with @ViewChild (something like InjectionToken<MdFormFieldControl> causes a type error). It doesn't have to be abstract though, I could use a concrete class with no-op methods.

jelbourn · 2017-08-08T00:48:59Z

src/lib/form-field/form-field.ts

+
+
+/** An interface which allows a control to work inside of a `MdFormField`. */
+export abstract class MdFormFieldControl {


Move this to its own file? Now that this is its own thing, we could probably break it up even more (prefix, suffix, error, and hint in their own files). That would bring this file down to that sweet, sweet 200 - 300 line range.

mmalerba · 2017-08-08T15:37:27Z

comments addressed

jelbourn · 2017-08-08T16:46:50Z

There are failures in the aot and e2e tests. Potentially need to regenerate the example module

mmalerba · 2017-08-08T18:09:28Z

@jelbourn Those failures appear to be due to the re-export of MdFormField as MdInputContainer in public_api.ts. Perhaps because I'm trying to export it under 2 different names?

edit: removed alias since it's problematic. I can update google's code when it's synced

crisbeto · 2017-08-08T18:02:05Z

src/lib/form-field/_form-field-theme.scss

+  $is-dark-theme: map-get($theme, is-dark);
+
+  // Placeholder colors. Required is used for the `*` star shown in the placeholder.
+  $form-field-placeholder-color: mat-color($foreground, secondary-text);


Since these are local variables, you can get rid of the form-field- prefix.

crisbeto · 2017-08-08T18:03:51Z

src/lib/form-field/form-field-control.ts

+  value: any;
+
+  /** Gets the element ID for this control. */
+  abstract getId(): string;


Perhaps this, getPlaceholder and getNgControl should be properties instead?

I just didn't want anyone trying to set them...

Could use readonly

crisbeto · 2017-08-08T18:04:21Z

src/lib/form-field/form-field-control.ts

+  stateChanges: Observable<void>;
+
+  /** The value of the control. */
+  value: any;


Instead of having any here, the MdFormFieldControl could be generic.

crisbeto · 2017-08-08T18:04:53Z

src/lib/form-field/form-field-control.ts

+   * Stream that emits whenever the state of the control changes such that the parent `MdFormField`
+   * needs to run change detection.
+   */
+  stateChanges: Observable<void>;


Perhaps allow this to be a Subject as well so we don't have to add asObservable?

I believe a Subject is a type of Observable? Just for the purposes of this interface we only care about its Observable features.

Having a Subject on the concrete class would satisfy it being an Observable

crisbeto · 2017-08-08T18:05:31Z

src/lib/form-field/form-field-control.ts

+  abstract getPlaceholder(): string;
+
+  /** Gets the NgControl for this control. */
+  abstract getNgControl(): NgControl | null;


Instead of NgControl | null this could be an optional property/method instead.

mmm, I think anyone who wants to implement this interface will want to include this method, its just that the method may sometimes return null if angular/forms isn't being used. (same rationale as above for not wanting a property).

crisbeto · 2017-08-08T18:08:20Z

src/lib/form-field/form-field.ts

+  // MdInput.
+  styleUrls: ['form-field.css', '../input/input.css'],
+  animations: [
+    trigger('transitionMessages', [


I know that this is just a refactoring, but maybe we should consider getting the placeholder animation to go through Angular as well? This allows users to disable it a bit easier.

👍 added a TODO

mmalerba · 2017-08-09T16:15:39Z

comments addressed

jelbourn

LGTM

crisbeto

LGTM

angular-automatic-lock-bot · 2019-09-08T16:02:56Z

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.}

mmalerba added the pr: needs review label Aug 7, 2017

mmalerba assigned jelbourn, crisbeto and tinayuangao Aug 7, 2017

mmalerba requested review from jelbourn, crisbeto and tinayuangao August 7, 2017 20:27

mmalerba requested review from amcdnl and kara as code owners August 7, 2017 20:27

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 7, 2017

mmalerba added 5 commits August 7, 2017 14:29

refactor all communication between input-container and input into a

863bd28

separate interface

rename input-container to form-field

b607297

alias mat-input-* classes with mat-form-field-* classes

bb2dca5

separate md-form-field into separate directory

dfddd97

rebase and fix lint

b16ef28

mmalerba force-pushed the input-select2 branch from 5099a97 to b16ef28 Compare August 7, 2017 21:44

mmalerba mentioned this pull request Aug 7, 2017

feat(select): add support for custom errorStateMatcher #6147

Closed

jelbourn reviewed Aug 8, 2017

View reviewed changes

mmalerba mentioned this pull request Aug 8, 2017

feat(select): add ability to customize the select trigger #3341

Merged

mmalerba force-pushed the input-select2 branch from 8e8440d to 48b3cc9 Compare August 8, 2017 15:02

address comments

9baaaeb

mmalerba force-pushed the input-select2 branch from 48b3cc9 to 9baaaeb Compare August 8, 2017 15:36

crisbeto reviewed Aug 8, 2017

View reviewed changes

mmalerba added 2 commits August 8, 2017 16:46

addressed comments

a1d5ce9

getter methods --> readonly properties

5a957bd

jelbourn approved these changes Aug 9, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Aug 9, 2017

crisbeto approved these changes Aug 9, 2017

View reviewed changes

mmalerba merged commit 0754320 into angular:master Aug 10, 2017

danieleds added a commit to danieleds/material2 that referenced this pull request Sep 3, 2017

chore: add MdInputDirective to breaking changes (angular#6331)

17627e6

mmalerba deleted the input-select2 branch April 3, 2018 15:19

angular-automatic-lock-bot bot unassigned tinayuangao Sep 8, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(input): rename `input-container` to `form-field`... #6331

refactor(input): rename `input-container` to `form-field`... #6331

mmalerba commented Aug 7, 2017 •

edited

Loading

mmalerba commented Aug 7, 2017

jelbourn Aug 8, 2017

jelbourn Aug 8, 2017

mmalerba Aug 8, 2017

jelbourn Aug 8, 2017

jelbourn Aug 8, 2017

mmalerba Aug 8, 2017

jelbourn Aug 8, 2017

mmalerba Aug 8, 2017

jelbourn Aug 8, 2017

jelbourn Aug 8, 2017

mmalerba Aug 8, 2017 •

edited

Loading

jelbourn Aug 8, 2017

mmalerba Aug 8, 2017

jelbourn Aug 8, 2017

mmalerba commented Aug 8, 2017

jelbourn commented Aug 8, 2017

mmalerba commented Aug 8, 2017 •

edited

Loading

crisbeto Aug 8, 2017

crisbeto Aug 8, 2017

mmalerba Aug 8, 2017

jelbourn Aug 8, 2017

crisbeto Aug 8, 2017

crisbeto Aug 8, 2017

mmalerba Aug 8, 2017

jelbourn Aug 8, 2017

crisbeto Aug 8, 2017

mmalerba Aug 8, 2017

crisbeto Aug 8, 2017

mmalerba Aug 8, 2017

mmalerba commented Aug 9, 2017

jelbourn left a comment

crisbeto left a comment

angular-automatic-lock-bot bot commented Sep 8, 2019



		/** An interface which allows a control to work inside of a `MdFormField`. */
		export abstract class MdFormFieldControl {

refactor(input): rename input-container to form-field... #6331

refactor(input): rename input-container to form-field... #6331

Conversation

mmalerba commented Aug 7, 2017 • edited Loading

mmalerba commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Aug 8, 2017

jelbourn commented Aug 8, 2017

mmalerba commented Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Aug 9, 2017

jelbourn left a comment

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 8, 2019

refactor(input): rename `input-container` to `form-field`... #6331

refactor(input): rename `input-container` to `form-field`... #6331

mmalerba commented Aug 7, 2017 •

edited

Loading

mmalerba Aug 8, 2017 •

edited

Loading

mmalerba commented Aug 8, 2017 •

edited

Loading