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

Move hand cursor for buttons to reboot #27021

Merged
merged 3 commits into from
Jan 4, 2019

Conversation

MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Aug 5, 2018

Fixes #26998, fixes #22693

  • This PR adds a hand cursor for all buttons
  • Adds $enable-hand-cursor-for-buttons variable which allows developers to still use the default cursor for all buttons.
  • Adds a reset button example to the reboot page
  • Moved all the separated declarations to scss/_reboot.scss.

Closes #25598

Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

This looks tidy to me (though I bet people still won't be happy, or start asking why we're not doing it for generic .btn applied to arbitrary elements as well...)

@ysds
Copy link
Member

ysds commented Aug 7, 2018

similar PR and discussion in #25598

Copy link
Collaborator

@andresgalante andresgalante left a comment

Choose a reason for hiding this comment

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

@MartijnCuppens Thanks a lot, this is a good move IMO.
I'd change enable-hand-cursor-for-buttons to enable-pointer-cursor-for-buttons, what do you think?

@patrickhlauke
Copy link
Member

@andresgalante yup, good catch...technically more accurate to say enable-pointer-cursor... (though i wonder if devs will be confused/not know that pointer in CSS actually results in the "hand")

scss/_reboot.scss Outdated Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
site/docs/4.1/getting-started/theming.md Outdated Show resolved Hide resolved
@MartijnCuppens
Copy link
Member Author

No problem, just changed it to $enable-pointer-cursor-for-buttons.

@patrickhlauke
Copy link
Member

👍 ... @mdo what say you?

scss/_reboot.scss Outdated Show resolved Hide resolved
@XhmikosR
Copy link
Member

@mdo: ping for review

Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

Current selectors include :not(.disabled) as well as the disabled attribute. I don't think we can consolidate all these under a single attribute selector, nor can we move away from the component classes completely I'd think since many, if not most, of these apply to anchors as well.

@jacobmllr95
Copy link
Contributor

Isn't this going to be available in v4.2?

@MartijnCuppens MartijnCuppens requested a review from a team as a code owner December 7, 2018 15:24
@MartijnCuppens
Copy link
Member Author

Current selectors include :not(.disabled) as well as the disabled attribute. I don't think we can consolidate all these under a single attribute selector, nor can we move away from the component classes completely I'd think since many, if not most, of these apply to anchors as well.

@mdo , this change won't affect the links, they will still have cursor: pointer unless a class .disabled is added to a.btn. The only thing this will change is that the button (and input buttons) will have cursor:pointer by default.

Copy link
Collaborator

@andresgalante andresgalante left a comment

Choose a reason for hiding this comment

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

I think we should move this forward

@mdo mdo mentioned this pull request Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants