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

style(disabled components): changed cursor to default when disabled - #5612

Closed
EladBezalel wants to merge 1 commit into
masterfrom
fix/switch-radio-disabled-cursor
Closed

style(disabled components): changed cursor to default when disabled#5612
EladBezalel wants to merge 1 commit into
masterfrom
fix/switch-radio-disabled-cursor

Conversation

@EladBezalel

Copy link
Copy Markdown
Member

Replaced all no-drop and not-allowed cursors to default

fixes#4831

@EladBezalelEladBezalel added the pr: merge ready This PR is ready for a caretaker to review label Nov 7, 2015
@EladBezalelEladBezalel added needs: review This PR is waiting on review from the team and removed pr: merge ready This PR is ready for a caretaker to review labels Nov 8, 2015
@EladBezalel
EladBezalelforce-pushed the fix/switch-radio-disabled-cursor branch from ef5eae8 to ba07079CompareNovember 13, 2015 13:33
@EladBezalelEladBezalel changed the title fix(switch, radio): changed cursor to no-drop when disabledstyle(disabled components): changed cursor to default when disabledNov 13, 2015
@EladBezalelEladBezalel added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Nov 13, 2015
@ThomasBurlesonThomasBurleson added the needs: team discussion This issue requires a decision from the team before moving forward. label Nov 13, 2015
@ThomasBurlesonThomasBurleson modified the milestone: 1.0-rc4Nov 13, 2015
@gkalpak

Copy link
Copy Markdown
Member

Why change it to default (and how is it related to #4831 ?
I prefer not-allowed (but #4831 still needs to be fixed).

@EladBezalel

Copy link
Copy Markdown
MemberAuthor

@gkalpak this is fixing #4831 by adding a cursor style

Take a look here, this is disabled native components codepen, as you can see the cursor is default

@gkalpak

Copy link
Copy Markdown
Member

Yes, adding a cursor style for disabled radios (or other disabled controls, e.g. select, switch) lgtm. But it doesn't have to be default, right ?

not-allowed conveys the "disabled-ness" (not a real word, I know) better imo, so why not use that ?

@EladBezalel

Copy link
Copy Markdown
MemberAuthor

@gkalpak this is why it's labeled as team discussion 😄

IMO our components should behave as the native components.

@jelbourn@ThomasBurleson@robertmesserle any thoughts?

@gkalpak

Copy link
Copy Markdown
Member

@EladBezalel, that's why we're discussing 😃
What I wasn't sure about (and that's why I adked), is what's the rational behind changing not-allowed to default (and how/if it relates to fixing #4831).

So, if I have understood correctly, there are 2 different things in this PR:

  1. Add some missing cursor styles for disabled radios, selects and switches (which fixes Disabled radio seems clickable with pointer cursor (hand) #4831).
  2. Change the cursor style for disabled controls to default (instead of not-allowed).

The former totally lgtm. The latter (which could go in a different PR imo) is what needs discussion, I guess 😄

Being consistent with the native controls makes sense. On the other hand, we are putting some considerable effort into looking much better than the native controls 😁
Since I wouldn't expect not-allowed to cause any confusion to users, I slightly favor it over default. (I admit it can appear as too "bold" in some occasions, but according to my experience they are the minority and the styles can be overwritten on a case-by-case basis.)

All in all, I am fine with either alternative, as long as the styles are consistent across controls.

So, let's hear what others have to say 👂

@SanderElias

Copy link
Copy Markdown
Contributor

I'm in favor for the default cursor option, for 2 reasons.

  1. it's in line with native controls
  2. when a user is just moving his mouse around, it is way less distracting.

@jelbourn

Copy link
Copy Markdown
Contributor

@EladBezalel Yes- the cursor should absolutely, 100% be default. LGTM.

@jelbournjelbourn removed the needs: team discussion This issue requires a decision from the team before moving forward. label Nov 16, 2015
@EladBezalel
EladBezalelforce-pushed the fix/switch-radio-disabled-cursor branch 2 times, most recently from 17d383f to a77f858CompareNovember 23, 2015 19:01
@EladBezalel
EladBezalelforce-pushed the fix/switch-radio-disabled-cursor branch from a77f858 to 69dc1afCompareNovember 28, 2015 22:45
@EladBezalel

Copy link
Copy Markdown
MemberAuthor

@ThomasBurleson rebased and ready to be merged

@EladBezalelEladBezalel modified the milestones: 1.0-rc6, 1.0-rc4Dec 2, 2015
@EladBezalel
EladBezalel deleted the fix/switch-radio-disabled-cursor branch December 3, 2015 17:36
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

pr: merge readyThis PR is ready for a caretaker to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disabled radio seems clickable with pointer cursor (hand)

5 participants

@EladBezalel@gkalpak@SanderElias@jelbourn@ThomasBurleson