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

v4: Add interaction utils #30562

Merged
merged 2 commits into from
May 11, 2020
Merged

v4: Add interaction utils #30562

merged 2 commits into from
May 11, 2020

Conversation

mdo
Copy link
Member

@mdo mdo commented Apr 11, 2020

This might be overstepping things for our next v4 release, but I noticed how highly requested the .cursor-pointer utility was in #23709 and couldn't help but stub out a PR to address it. I've taken the v5 approach of an interactions page and included this new util, and the user-select one from v5. Decided not to include pointer-events utils because of browser compatibility.

I'd really like to ship small enhancements to v4 for folks with highly requested features like this, especially if it helps bridge the gap between v4 and v5. My biggest question is how we version/release this though.

Do we squeeze this into a v4.4.2 despite of it being a new feature and thus requiring a semver minor bump? Or do we upgrade v4.4.2 to v4.5?

@mdo mdo requested a review from a team as a code owner April 11, 2020 20:37
This was referenced Apr 11, 2020
@XhmikosR XhmikosR force-pushed the v4-interaction-utils branch from 65426ff to 8e05fca Compare April 12, 2020 11:23
@XhmikosR
Copy link
Member

Squashed the patches.

TBH I'd prefer it if we didn't make it a new minor release. On the other hand, semver...

Let's wait for the others to chime in.

@MartijnCuppens
Copy link
Member

I'd prefer it if we didn't make it a new minor release

I agree

@mdo
Copy link
Member Author

mdo commented Apr 30, 2020

Revisiting this, if @twbs/css-review is into it, I'm inclined to sneak this into v4.4.2 to help close the gap between v4 and v5. Really think this will be some solid good will for our users :).

@XhmikosR
Copy link
Member

We never followed semver 100% and this, at least, is not a breaking change, so I think we can add it.

@MartijnCuppens
Copy link
Member

I'm gonna stick with what I said in #30563 (comment). I don't want people to abuse this property.

What I am willing to consider is adding

[role="button"] {
  cursor: pointer;
}

to reboot since this will be required anyway.

@XhmikosR
Copy link
Member

That's a valid concern of course, but on the other hand, one can buy a gun legally (in some countries); if they kill someone the store cannot be held liable for any murders. 😇

But I definitely see your point. Maybe if we added a warning callout it'd be better?

@ffoodd
Copy link
Member

ffoodd commented May 6, 2020

I'd be on both sides on this one:

  • I agree with @MartijnCuppens about cursors and don't think it should be a utility at all. I already know tons of people who'd just add a div with .btn things and .cursor-pointer—and it'll probably be bad. [role="button"] seems more accurate.
  • user-select on the other hand seems handy, I don't see any weird use cases (but I may be a bit naive on this).

@mdo mdo force-pushed the v4-interaction-utils branch from 1ab721a to 740ca9a Compare May 7, 2020 16:58
@mdo
Copy link
Member Author

mdo commented May 7, 2020

Updated to reflect the team's feedback! Thanks y'all.

@ffoodd
Copy link
Member

ffoodd commented May 10, 2020

Shouldn't [role="button"] sit in reboot, as suggested by @MartijnCuppens? I think it shouldn't be considered as a utility since it needs accessibility and scripting considerations to ne used the right way.

Maybe even provide a sentence with explanation or link?

scss/utilities/_interactions.scss Outdated Show resolved Hide resolved
site/docs/4.4/utilities/interactions.md Outdated Show resolved Hide resolved
- Adds .user-select-* utils from v5
- Adds button role attribute util, one of the top requested features in our issues for adding pointer cursors
- Adds new docs page to demonstrate both
- Includes Sass list for customizing user select
@mdo mdo force-pushed the v4-interaction-utils branch from 740ca9a to 86e65d4 Compare May 11, 2020 16:56
@mdo
Copy link
Member Author

mdo commented May 11, 2020

Think I got the feedback applied @MartijnCuppens and @ffoodd. Let me know with any suggested changes or review comments if there's something I can clarify more in the docs <3.

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

LGTM. We'll probably add some warning about the button role usage, but I think it's more @patrickhlauke's business here.

Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

:shipit:

@XhmikosR XhmikosR merged commit 5d10e4a into v4-dev May 11, 2020
@XhmikosR XhmikosR deleted the v4-interaction-utils branch May 11, 2020 17:57
@XhmikosR
Copy link
Member

FYI the utilities/interactions.scss wasn't included in utilities.scss. I fixed this in my v4-dev PR #30768.

XhmikosR added a commit that referenced this pull request May 12, 2020
XhmikosR added a commit that referenced this pull request May 12, 2020
XhmikosR added a commit that referenced this pull request May 12, 2020
XhmikosR added a commit that referenced this pull request May 12, 2020
marcop135 added a commit to marcop135/bullframe.css that referenced this pull request May 13, 2020
mdo added a commit that referenced this pull request Jun 16, 2020
Ports the changes from #30562 made in v4.5 and adds them to v5. This replaces #30563 which sought to add this to the utility API, but the v4 PR shifted to implement an accessible solution vs a lone utility.
mdo added a commit that referenced this pull request Jun 16, 2020
* v5: Add role=button cursor in Reboot

Ports the changes from #30562 made in v4.5 and adds them to v5. This replaces #30563 which sought to add this to the utility API, but the v4 PR shifted to implement an accessible solution vs a lone utility.

* Update reboot.md

Co-authored-by: XhmikosR <xhmikosr@gmail.com>
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
* v5: Add role=button cursor in Reboot

Ports the changes from twbs#30562 made in v4.5 and adds them to v5. This replaces twbs#30563 which sought to add this to the utility API, but the v4 PR shifted to implement an accessible solution vs a lone utility.

* Update reboot.md

Co-authored-by: XhmikosR <xhmikosr@gmail.com>
marcop135 added a commit to marcop135/bullframe.css that referenced this pull request Mar 12, 2021
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.

4 participants