Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

fix(select): Fix empty option stlying issue. #8907

Conversation

topherfangio
Copy link
Contributor

@topherfangio topherfangio commented Jun 30, 2016

Previously, if you used ng-value on your md-option and attempted to have an empty option without a value (i.e. <md-option>Select...</md-option>), the component would still be in a "has selected value" state causing the floating label to appear among other issues.

Add a new md-option-empty attribute to <md-option> which allows the user to specify the empty option. Additionally, automatically add this attribute for the most common use cases.

Update demo/docs to show usage.

Fixes #6851.

@topherfangio topherfangio added the needs: review This PR is waiting on review from the team label Jun 30, 2016
@topherfangio topherfangio added this to the 1.1.0 milestone Jun 30, 2016
@clshortfuse
Copy link
Contributor

At a glance, it looks good, but in the demo, is there no text for the blank option? Is it possible to do an italicized ’(None)‘ inside?

@clshortfuse
Copy link
Contributor

@bradrich
Copy link
Contributor

Is there a reason why you decided to use the absence of the ng-value or value attribute to be the trigger for clearing the select model?

The common method is to pass an empty or undefined value. My thinking is that the alteration could be made to the inputCheckValue function we were speaking about before.

What do you think?

@bradrich
Copy link
Contributor

I am looking at the difference between a <md-option value></md-option> and a <md-option ng-value="something">{{something}}</md-option> and how to test it.

One difference is the selectMenuCtrl.selectedLabels() of the option with an empty value. When it is selected it is <div class="_md-text"></div>. An option with a value is <div class="_md-text ng-binding">Something</div>. The empty value one contains no ng-binding. However, this is a poor method of testing.

My suggestion is to check if the value of selectMenuCtrl.selected[Object.keys(selectMenuCtrl.selected)[0]] and if it is undefined, null, or an empty string. In the case of this code:

<md-input-container style="width: 200px;">
  <label>State ({{ctrl.userState2 === null || ctrl.userState2 === undefined || ctrl.userState2 === '' ? 'undefined' : ctrl.userState2}})</label>
  <md-select name="state" ng-model="ctrl.userState2">
    <md-option value></md-option>
    <md-option ng-repeat="state in ctrl.states" ng-value="state.abbrev">{{state.abbrev}}</md-option>
  </md-select>
</md-input-container>

If ctrl.userState2 is not defined in the controller, the value of selectMenuCtrl.selected[Object.keys(selectMenuCtrl.selected)[0]] is undefined initially. If you set it to null in the controller, then it is null. When you select a value and then re-select the empty value, then it is an empty string. You could then pass the checks for these into the containerCtrl.setHasValue().

@bradrich
Copy link
Contributor

This works when replacing https://github.com/angular/material/blob/master/src/components/select/select.js#L481:

var selectedValue = selectMenuCtrl.selected[Object.keys(selectMenuCtrl.selected)[0]];
containerCtrl && containerCtrl.setHasValue(
  (selectedValue !== undefined && selectedValue !== null && selectedValue !== '' && selectMenuCtrl.selectedLabels().length > 0) || (element[0].validity || {}).badInput
);

@topherfangio
Copy link
Contributor Author

@clshortfuse If the option has text (i.e. "None"), then I would expect that text to appear in the input and move the floating label up to make room. In it's current state, it would not do that. If you add "None" as text, and then select that option, it would clear the element and drop the floating label back down.

That said, I don't think there is anything stopping someone from doing whatever approach they like since you CAN add "None" if you want.

@bradrich Aside from "that's the way the HTML version works", is there an advantage to using a blank value attribute instead of simply not providing one? The disadvantage I see of forcing it to be one of (or multiple of) value=undefined (which is the same as just <md-option value>), value=null, or value='' is that you can no longer use those as valid/selectable options since they would clear the select.

I am certainly open to thoughts/changes to this. I'd like to do it the "right" way 😄

Previously, if you used `ng-value` on your `md-option` and attempted
to have an empty option without a value
(i.e. `<md-option>Select...</md-option>`), the component would still
be in a "has selected value" state causing the floating label to
appear among other issues.

Add a new `md-option-empty` attribute to `<md-option>` which allows
the user to specify the empty option. Additionally, automatically
add this attribute for the most common use cases.

Update demo/docs to show usage.

Fixes angular#6851.
@topherfangio topherfangio force-pushed the fix-select-undefined-value-6851 branch from 11d205e to b9eb9d6 Compare July 4, 2016 22:50
@topherfangio
Copy link
Contributor Author

topherfangio commented Jul 5, 2016

@clshortfuse @bradrich Made some updates to this PR, it is ready for re-review.

@clshortfuse
Copy link
Contributor

@topherfangio Look good, though I think you should add some tests related for md-option-empty.

@@ -723,6 +749,11 @@ function SelectMenuDirective($parse, $mdUtil, $mdTheming) {
// Map the given element to its innerHTML string. If the element has a child ripple
// container remove it from the HTML string, before returning the string.
mapFn = function(el) {
// If we do not have a `value` or `ng-value`, assume it is an empty option which clears the select
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems to be left over from old code. Needs updating or removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed; good catch!

@bradrich
Copy link
Contributor

bradrich commented Jul 5, 2016

@topherfangio LGTM and ditto on the speedy @clshortfuse who beat me to that comment comment!

@clshortfuse
Copy link
Contributor

@topherfangio i updated your comment accidently

Quick edit: I wouldn't abbreviate parameters to params in the documentation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: review This PR is waiting on review from the team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants