-
-
Notifications
You must be signed in to change notification settings - Fork 79k
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
Add variable for disabled cursor #14891
Conversation
Hm... this actually removes the not-allowed cursor from all kinds of things. I'm not saying it's wrong, but it's a change in behavior rather than a fix. The suggestion I made in #14882 was to just change it in buttons.less, which only changes the behavior in older browsers (and changes it to match newer browsers). The problem I described no longer exists after this, but it is a bigger change than just that fix. (Still, I'm not necessarily against this, just stating that it does more.) |
I didn't mean that this is only a fix, it's definitely more than that :). The thing is, this has come up before, and I'd rather be consistent in how we apply disabled styles across the entire library. If there's no cursor or a not-allowed cursor in one place, it should be the same everywhere. |
Well, browsers already differentiate between |
Ah, yeah, good point @cvrebert: http://jsbin.com/demin/1/. Thinking about it more, should we just skip this altogether and go with what you said in #14882 (comment)?
The variable and cursor change aside, I'm in agreement here. Showing a cursor in one browser and not in all the others seems fine by me. |
Your call man. 😄 |
default
Sticking with |
I don't see any reason not to fix the browser inconsistency. As I see it, there are three things here:
I think we all agree 1 is a definite improvement, and for that reason it has now been merged. The question is whether to do 2, 3 or neither. I'd say 2 is an improvement over the current situation (and thus better than doing nothing). Behaviorally, it changes nothing for proper browsers, and makes sure older browsers have the same behavior as newer ones. On top of that, it actually makes the css smaller, so there's no disadvantage there either. The only downside I see is that the css isn't quite as consistent between different (form-)elements. However, I think the behavior is what should drive the change here and the behavior was different anyway, so I don't think this is a strong argument for not doing 2 (nor doing 3). In fact, I think having a statement in your css that has no effect (in proper browsers) is confusing (and a code smell), so even on codelevel this would be an improvement. The other option is to go for 3. I was never against this. I just meant to point out it was a functional change, since I like to be clear and not hide a functional change behind a problem that can be fixed without a functional change. Finally, there's another option I didn't consider until now, which is doing both 2 and 3. I think this actually makes more sense than just doing 3. The usecase of the variable is to allow users to easily change the cursor that is shown on disabled inputs. For example, say I decided that all my disabled inputs should show a At the end of the day, I'll learn to live with overwriting this in my own code if it is decided not to fix this, but I just don't understand why one wouldn't fix it... |
The problem is in nearly each version of IE below 11, you're dealing with another browser inconsistency. Accounting for individual problems like this is fine, but at some point it's fine to leave them as-is. For example, IE9 has a shitty text rendering problem for disabled buttons. IE8 doesn't render We can revisit in v3.3.1, mind you. If you feel that strongly about it, consider opening a pull request with the exact changes you'd like :). |
I believe there's a difference between getting something that is not quite the same but still looks decent and works well (graceful degradation) and having something new show up in an older browser that would usually be covered up by a feature that is not supported by this browser but only changes the look of things. Perhaps my view is somewhat tainted by the fact that I received feedback on a situation where a problem in our own code made more things happen on mouseover in IE 10. I'll look into the PR some time in the future. I'm just used to the idea that "won't fix" means that it's not going to be fixed, but here I suppose it meant that it wasn't going to be fixed for now but could possibly be in the future, especially if there is a PR for it. |
Currently our disabled elements use
cursor: not-allowed;
which on hover shows a circle with a slash through it. It's intended to show the click is, well, not allowed, but it's not something folks see all that often I feel. In addition, we don't currently have an easy way to customize this should you want to adjust it.So, this PR adds the variable and sets it todefault
instead ofnot-allowed
. This also has the added benefit of fixing a recent inconsistency in behavior—most modern browsers show no cursor at all due topointer-events
. This brings IE10 and below closer in line with that.Edit: Alright, screw that, we're going with
not-allowed
still and adding the variable./cc @cvrebert @JasperHorn