Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Treat range inputs the same as number inputs #2085

Closed
wants to merge 2 commits into from

Conversation

chrisnicola
Copy link
Contributor

I've simply added range as to the list of input types that can be treated as a number type. It has the same properties (min, max) and should be capable of behaving exactly the same way.

I'm not too comfortable with the docs/examples yet so I may have done something wrong. Please let me know if I need to fix anything.

This fixes issue #1189

@pkozlowski-opensource
Copy link
Member

PR Checklist (Minor Feature)

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
  • Feature improves existing core functionality
  • API is compatible with existing Angular apis and relevant standards (if applicable)
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

PR specific todos:

  • handle situation when model is undefined or null
  • handle situation when model is set to a value outside of the allowed range
  • handle situation when model is set to an invalid value (non-numeric)
  • handle situation when model was set to a valid number, but later was set to undefined or null

@pkozlowski-opensource
Copy link
Member

@LucisFerre Thnx for submitting this PR. It will require some work before it can be merged. Most notably we are missing unit tests and documentation update. You will have to sign the CLA and squash commits / update commit message as well. I've included a checklist for you.

@vvakame
Copy link

vvakame commented Mar 6, 2013

👍 I need range input too :)

@chrisnicola
Copy link
Contributor Author

I have my CLA in. Sorry this week is terrible for me. This weekend I'll finish the other items on the list. I'm reluctant to submit it without a seperate PR for allowing the min/max validations to work with dynamically set min/max values. This isn't as big an issue for a regular number field, but for a range field it can make it behave pretty screwy.

Either that or not use min/max validation for range fields (assuming there is UI support the UI typically handles this).

There are a couple of incomplete PRs on it so far.

#1077
#666

Looks like neither were really finished properly, so I may submit a new one after looking at them.

@ghost ghost assigned IgorMinar Mar 12, 2013
@IgorMinar
Copy link
Contributor

This is actually a bigger job than what it looks like because of all the corner-cases surrounding the range input control as implemented in todays browsers.

Take this for example: http://plnkr.co/edit/uL2iyT2WwZJc7M7Stm55?p=preview

in that app I'm just rendering range without setting the value to anything, when you look at the output you'll see that:

  • the slider thumb in the component is positioned in the middle of the slider
  • the value of as read from the dom is set to '50'

Now consider these scenarios:

  • let's say our model is null or undefined, if we don't update the range control it will be rendered with the slider thumb in the middle - this seems wrong
  • let's say that our model is set to 60 and angular updates the range control, then we set the model to undefined OR value outside of the allowed range OR non-numeric value - the slider will either remain at the old position or will be reset to point to the middle - this seems very wrong

how can we deal with these corner-cases? from reading the range spec I don't see any special state that would allow us to represent these scenarios (which IMO is spec deficiency).

@mhevery and I talked about this and we think that ideally the thumb should disappear if we can't set the slider thumb to a value that accurately represents the current model value. Can we make the thumb disappear? Maybe, using css3 selectors like when styling the scroll bar, we could be able to make the thumb disappear.

The tumb should reappear with either user clicks anywhere in the slider (and selects a valid value in that way) or when the model is set to a valid value.

Does anyone have a better suggestion? Is anyone willing to experiment with this more?

@chrisnicola
Copy link
Contributor Author

@IgorMinar I don't agree. I can't think of any slider that would do that (disappear for having no value). Generally speaking a slider should be required to have a value, if the model has no value and gets bound to a range I would suggest initializing it to 0 or the minimum value. I would treat any "range" input as "required" otherwise they should just use a number.

If no value is an option and the designer wants a slider then they would have to also have an toggle, like a checkbox, for graying it out which would need to unbind that slider from the value (or something like that).

The problem I find with corner cases is that almost no one spends any time in those corners and they aren't very important.

Bottom line, if the developer does something stupid here I'm ok with stupid things happening to them.

@chrisnicola
Copy link
Contributor Author

Ok re-reading and that comes across as snarky. Sorry about that. All I'm trying to say, is it's fine not to handle this corner case in any special way. I will experiment a bit more and try to get something done this week but I don't plan on striving for perfection.

@IgorMinar
Copy link
Contributor

@LucisFerre it's not that simple. in angular the view should never drive the model, instead the model should drive the view. so if the slider says that it's at 0 (or 50) and model is undefined then this is really weird and will result in unexpected behavior of the app.

All these cornercases are important because we deal with them for all the other form controls (e.g. select box). So we can't just wave our hand and say that the behavior in these cases is undefined.

@chrisnicola
Copy link
Contributor Author

in angular the view should never drive the model, instead the model should drive the view

@IgorMinar that's a pretty restrictive attitude. The fact is you are designing a UI, the design of the interface does in fact dictate desired behavior. If I use a slider but then bind it to some data that makes no sense for sliders (like text data) then I'm going to "have a bad time". Same goes for binding it to nothing. This isn't a real corner case for the UI, it's a failure case for the design.

Besides, it's be pretty easy to argue that the view does in fact drive the model, if by model we mean the "scope". Angular's scopes are essentially "view-models" by the very nature of two-way binding and are really more a model (and perhaps extension) of the view than a domain model. The domain model typically will live in the services so it is completely decoupled from the view instead of tightly bound.

@chrisnicola
Copy link
Contributor Author

Alright one more try. Basically if the designer/developer wants to define a state for their UI which handles no value for the slider that is up to them and it's easy enough for them to decide how they want to handle this with Angular.

Now they may want to handle it the way you suggest (hide the thumb if that is actually possible with CSS) or they may want to handle it entirely differently (hide or gray the slider). That really is up to them. I don't see why we would decide for them though. It would be one thing if the browser sliders supported a concept like this themselves but they don't. You are suggesting introducing a hack for this case, one that may work for some but not for others.

@ganarajpr
Copy link

@IgorMinar I know for a fact that an input type number also has restrictions that it cannot have an input that is non numeric right? This is a restriction that angular applies on top of the browser as well ( I had an issue with this recently, since a number input has default number validation which is applied before pushing it into the model and we did not want that, but I digress ).
I would actually vote for resetting the value to the minimum when the model value goes outside the bounds since as a user that makes the most sense to me. First of all, are there any scenarios where you would two way bind a range input to another input field? Unless its a demo app it doesn't make sense to do that.

@chrisnicola
Copy link
Contributor Author

@ganarajpr sadly I have to admit that scenario isn't that uncommon. I've actually seen UIs that do this. The range input provides improved usability for touch devices but they also provide the text input for keyboard users and, in some cases, for entering a value that goes outside of the expected range (yes I'm not sure I agree with the UX but I've seen it done). Obviously it is impossible to account for every design, however, it is enough to manage these unusual designs in the code (imo).

What I'm worried about is that over-engineering the binding of a range input will hinder designers rather than help them.

@chrisnicola
Copy link
Contributor Author

@IgorMinar I'm revisiting this again now sorry about the delay.

You can hide the range slider thumb if necessary with some CSS. Something like this works just fine.

input[type="range"]::-webkit-slider-thumb { opacity: 0; }

However because the thumb is a DocumentFragment I'm not aware of any way to apply the style directly to it. I'm also not sure if I like the idea of doing that anyways as it would be hard to override the style. What we could do is add a CSS class into the styles in order to deal with this.

Though I'll admit my preference here is to just use disabed="disabled", while it doesn't hide the thumb it does disable the element, and the attribute makes it easy enough for the developer to apply a style that hides the thumb if that's what they prefer. Similarly, if they want it to have an enable on-click behaviour that assigns a value that is easy enough too.

Thoughts? I'm just not sure what the most consistent approach for AngularJS is as far as styling things for people.

@petebacondarwin
Copy link
Contributor

@IgorMinar - Having the thumb disappear or disabling the range input seems like a bad idea to me. Programmatically setting an invalid value should not prevent the user from then interacting with the widget.

Not sure where this leaves us with the current discussion...

@petebacondarwin
Copy link
Contributor

I think if you programmatically provide an invalid model value then the input[range] directive should simply apply the ng-invalid class to the element and leave the developer to decide how to display that to the user.

@petebacondarwin
Copy link
Contributor

Here is a playground: http://plnkr.co/edit/T7kd0wGMKGs3jLLofi0I?p=preview

@chrisnicola
Copy link
Contributor Author

@petebacondarwin I like that, I agree letting the developer decide how to style is the simplest approach for the time being. I do also understand that making obvious cases easier is a good practice, however right now I'm not sure what the obvious cases with invalid model values are.

@petebacondarwin
Copy link
Contributor

@LucisFerre - either way, I don't think that this is going to be merged very soon. I would recommend converting your code into an external directive. You could push it into angular-ui project then get it some exposure. If people start using it then you'll be able to see the use cases and also they will raise any UX issues. Then once it has a while to mature in the wild it would be time to integrate it into the core.

I am happy to help you get started in AngularUI if you fancy going that way. Alternatively, simply create your own project and bower component then let it loose!

@petebacondarwin
Copy link
Contributor

Going to close this as it is not going to be merged for now unless a better design solution is found.

@shyndman
Copy link

Any movement on this issue?

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

Successfully merging this pull request may close these issues.

7 participants