feat(select): make select work inside form-field #6488

mmalerba · 2017-08-15T23:34:16Z

Demos:
https://mmalerba-demo3.firebaseapp.com/baseline
https://mmalerba-demo3.firebaseapp.com/select

BREAKING CHANGES:

md-select must now be wrapped in an md-form-field
MdFormFieldControl now has 2 new properties: controlType, shouldPlaceholderFloat and 1 new method: onContainerClick

kara · 2017-08-16T00:30:43Z

@mmalerba Great work! I played around the demo. It seems mostly good, but one thing I noticed is that sometimes you can click on the trigger without actually opening the panel. It floats the label, but the panel stays closed. If you try to click closer to the underline or the floating label, you can reproduce.

crisbeto

Code-wise LGTM, but I'm not too sure about the API.

crisbeto · 2017-08-16T16:59:28Z

src/demo-app/baseline/baseline-demo.html

-    <md-select placeholder="Select">
-      <md-option value="option">Option</md-option>
-    </md-select>
+    <md-form-field>


API-wise, do we really want people to have to wrap every select in a form field? E.g. if you were using option groups, you'd have to write 4 levels of HTML. Aside from having to proxy a few @Input-s, what are the cons of having the form field be consumed by md-select internally?

In the past we've preferred composition style APIs since they're more flexible, even if they are a bit more verbose. I think the extra typing is worth it, but it's a fair point. @jelbourn do you have an opinion?

I do like the consistency of having both select and input inside the same form-field; it makes it clear that the prefix, suffix, hint, and error belong to the form field. Very explicit, no magic. It's also more maintainable for us, since we don't have to forward along every form-field API through select over time.

I don't feel incredibly strongly about it, though. @mmalerba up to you if you think it would be the same amount of work to do it the other way around; it would certainly make the migration easier

crisbeto · 2017-08-16T17:02:21Z

src/lib/select/select.ts

@@ -890,31 +893,29 @@ export class MdSelect extends _MdSelectMixinBase implements AfterContentInit, On

  /** Calculates the scroll position and x- and y-offsets of the overlay panel. */
  private _calculateOverlayPosition(): void {
+    this._triggerHeight = this.trigger.nativeElement.getBoundingClientRect().height;
+    this._triggerFontSize = this._measureFontSizeEl.nativeElement.getBoundingClientRect().height;


I'm not sure that this would be completely accurate. Why not use getComputedStyle?

Do you know if getComputedStyle is guaranteed to give me a px value for font-size (as opposed to em, %, etc)? That was my main motivation for not doinggetComputedStyle. Also, I assume getComputedStyle is no worse from a performance perspective?

I'm pretty sure that you'll get a pixel value no matter what. Should be pretty easy to verify. As for performance, it should be pretty close to the getBoundingClientRect call.

mmalerba · 2017-08-16T22:21:02Z

@kara good catch, I fixed the focus and click behavior and also corrected the 1px error in placement of the multi-select dropdown

rafaelss95 · 2017-08-19T02:06:24Z

src/lib/input/input.spec.ts

@@ -515,36 +516,36 @@ describe('MdInput without forms', function () {
    fixture.detectChanges();

    let inputEl = fixture.debugElement.query(By.css('input')).nativeElement;
-    let labelEl = fixture.debugElement.query(By.css('label')).nativeElement;
+    let formFieldEl = fixture.debugElement.query(By.css('.mat-form-field')).nativeElement;


@mmalerba Taking advantage of the fact that you're working on it, would you be able to replace, let by const when possible? It seems that (almost) all the variables here (in whole test file) aren't being reassigned and still are declared as let.

This sounds like something we should do as part o a separate cleanup pass of the whole project, I'm sure there are tools that can analyze and fix this for us

mmalerba · 2017-08-21T23:16:57Z

@kara @jelbourn @crisbeto this is ready for more through review now

…rlapping the trigger arrow

code-tree · 2017-09-26T04:27:14Z

The \xa0 fix doesn't appear to work with AOT?

code-tree · 2017-09-26T04:28:17Z

src/lib/select/select.html

+      TODO(mmalerba): &nbsp; is currently broken in components with preserveWhitespace: false, so we
+      evaluate it as a JS string binding instead. Change back to &nbsp; once it works again.
+    -->
+    <ng-container *ngIf="empty">{{'\xa0'}}</ng-container>


This doesn't appear to work with AOT.

udayvunnam · 2017-09-27T10:42:41Z

@mmalerba The Demos are great. Can you be able to provide example code of this?

joseluisq · 2017-09-28T13:38:49Z

Awesome!
Definitely I can't wait for the next release!

angular-automatic-lock-bot · 2019-09-08T16:03:05Z

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 15, 2017

mmalerba assigned jelbourn, kara and crisbeto Aug 15, 2017

mmalerba requested review from jelbourn, kara and crisbeto August 15, 2017 23:34

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

kara removed their assignment Aug 16, 2017

crisbeto reviewed Aug 16, 2017

View reviewed changes

mmalerba force-pushed the select branch from 44f47ba to 1030687 Compare August 16, 2017 21:42

mmalerba force-pushed the select branch from 6c05821 to 1a28317 Compare August 17, 2017 18:06

mmalerba mentioned this pull request Aug 17, 2017

Placeholder overflow is hidden now #5865

Closed

mmalerba force-pushed the select branch from 4ec6a06 to f7a8004 Compare August 18, 2017 00:35

willshowell mentioned this pull request Aug 18, 2017

Feature: md-error for md-select #5500

Closed

mmalerba mentioned this pull request Aug 18, 2017

mat-select-underline in google chrome #5312

Closed

mmalerba force-pushed the select branch 2 times, most recently from 96cdb5e to b9bef76 Compare August 19, 2017 00:17

rafaelss95 reviewed Aug 19, 2017

View reviewed changes

willshowell mentioned this pull request Aug 21, 2017

md-select cannot be placed inside md-input-container, hence labels position are not same as in an md-input #6571

Closed

mmalerba force-pushed the select branch 3 times, most recently from 5411bf7 to c1af22d Compare August 21, 2017 23:14

mmalerba requested review from amcdnl and andrewseguin as code owners August 22, 2017 00:06

mmalerba force-pushed the select branch 2 times, most recently from 22e1182 to 218c98b Compare August 22, 2017 22:55

mmalerba added 19 commits September 21, 2017 12:12

open select when form field is clicked

fd27cd1

separate the "floating" logic from the "empty" logic

face17e

fix slight error in multi-select dropdown placement

327c04b

exclude hint and error from focusing select on click

9626eb1

add some fancy form-field features to select demo

d1132f2

add a class to form-field depending on the control type for easy styling

2d4cb62

fix most tests, exceptions noted with TODOs

b660ddf

fix prerender and input test

e0d2c06

make placeholder float when select is opened, and prevent it from ove…

07e15a2

…rlapping the trigger arrow

fix remaining tests

db019d2

fix lint

9c9e932

clean up measurement logic

6ec117e

fix rebase issues

9cc3837

fix remaining demos and examples

9c3f905

fix some firefox issues

c8fadfd

ignore rounding error in IE

6348841

update new select instance that was added

02cd9d3

update md-chip-list implementation of MdFormFieldControl

16b2172

fix   issue

11e7549

mmalerba force-pushed the select branch from 4daa8aa to 11e7549 Compare September 21, 2017 19:20

jelbourn merged commit d914cc4 into angular:master Sep 21, 2017

willshowell mentioned this pull request Sep 25, 2017

vertical alignement #7290

Closed

code-tree reviewed Sep 26, 2017

View reviewed changes

willshowell mentioned this pull request Sep 29, 2017

body font size changes leaved md-input and md-select placeholder fonts inconsistent #7422

Closed

mmalerba deleted the select branch April 3, 2018 15:19

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

feat(select): make select work inside form-field #6488

feat(select): make select work inside form-field #6488

mmalerba commented Aug 15, 2017 •

edited

Loading

kara commented Aug 16, 2017 •

edited

Loading

crisbeto left a comment

crisbeto Aug 16, 2017

mmalerba Aug 16, 2017

jelbourn Aug 23, 2017

crisbeto Aug 16, 2017

mmalerba Aug 16, 2017

crisbeto Aug 17, 2017

mmalerba commented Aug 16, 2017

rafaelss95 Aug 19, 2017 •

edited

Loading

mmalerba Aug 21, 2017

mmalerba commented Aug 21, 2017

code-tree commented Sep 26, 2017

code-tree Sep 26, 2017

udayvunnam commented Sep 27, 2017

joseluisq commented Sep 28, 2017

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

feat(select): make select work inside form-field #6488

feat(select): make select work inside form-field #6488

Conversation

mmalerba commented Aug 15, 2017 • edited Loading

kara commented Aug 16, 2017 • edited Loading

crisbeto left a 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 16, 2017

rafaelss95 Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Aug 21, 2017

code-tree commented Sep 26, 2017

Choose a reason for hiding this comment

udayvunnam commented Sep 27, 2017

joseluisq commented Sep 28, 2017

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

mmalerba commented Aug 15, 2017 •

edited

Loading

kara commented Aug 16, 2017 •

edited

Loading

rafaelss95 Aug 19, 2017 •

edited

Loading