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

show placeholders for select fields also when deselecting an option #165

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

hutzelknecht
Copy link

this will not only show the placeholder once (initially), but also after deselecting an option for SelectFields with allowBlank=true

@bvaughn
Copy link
Owner

bvaughn commented Oct 12, 2015

Interesting.

The current behavior was intentional. If an empty selection is allowed (aka a valid choice) then it seems wrong to re-prompt a user (with a placeholder) after he has already made his selection (the empty value).

Can you describe the user flow you're going for with this change to help me better understand?

@bvaughn bvaughn self-assigned this Oct 12, 2015
@hutzelknecht
Copy link
Author

In my use case i have two interdependent fields. Say one for "vehicle makes" and on for "vehicle models". Each time the "make field" changes the "models field" gets erased, therefore i need to set allow-blank as an attribute to the directive of the "model field". What i need is a placeholder text telling the user to select a value, because the "models field" is reset (kind of) and requires an input.

@bvaughn
Copy link
Owner

bvaughn commented Oct 12, 2015

Ah, I see. I assume a label above the select wouldn't work for this? (Have to ask!) :)

I'm really reluctant to mess much with the flow of selects, but since yours is an opt-in change- it's probably okay. Give me a little time to play around with it at some point today and I think I'm okay with merging it.

@bvaughn
Copy link
Owner

bvaughn commented Nov 8, 2015

Very sorry for taking so long to visit this PR. I stay really busy lately and it's a bit of a struggle to keep on top of my various open source projects sometimes.

That being said, I've been testing this change locally this afternoon and it doesn't work like I expect. Other boolean attributes are evaluated as true just by being present (eg. allow-blank) but this one requires a boolean value. Not a big deal but I'm not a fan of inconsistencies unless there's a good reason. Did you foresee this value being changed dynamically? (It seems like kind of a static thing to me.)

Another thing that seems unintentional is that the placeholder label shows as an option in the select. (It looks like any other option. This is a little confusing.) I think it should only be shown when the selected value is empty. If a non-empty value is selected- and allow-blank is true- the placeholder should not be shown. We can fix this by changing...

if ($scope.model.bindable && !$scope.showPlaceholderForEmptyValues) {

...to this...

if ($scope.model.bindable) {

What are your thoughts to the above?

@hutzelknecht
Copy link
Author

hi Brian,

you're right, the attribute should be treated the same as allowBlank:

$scope.showPlaceholderForEmptyValues = $attributes.hasOwnProperty('showPlaceholderForEmptyValues');

the behaviour i want for the select field is actually to display a placeholder only in the input field, not in the options. The way everything works right now is that only items in the options can be displayed in the input field. I'll try to figure out how i can make this work. meanwhile i'll update the line above.

@hutzelknecht
Copy link
Author

we could change the template for this to work. instead of ng-options, we would need:

<option ng-repeat="option in bindableOptions" ng-attr-style="{{option.default}} && 'display:none;'"  ng-attr-default="{{option.default}}" value="{{option[valueAttribute]}}">{{option[labelAttribute]}}</option>

and add the default flag to $scope.emptyOption

if ($scope.showPlaceholderForEmptyValues) $scope.emptyOption.default = true;

Then we would have to set display:none if the value is not empty. I'll give this one a try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants