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

Differentiate hover state for outline buttons #28999

Closed
wants to merge 16 commits into from
Closed

Conversation

mermop
Copy link

@mermop mermop commented Jul 9, 2019

The :hover state for outline buttons is effectively the same as the :active state, so it can be difficult to tell the difference between a button that is hovered over and one that is active - particularly when using eg checkbox/radio buttons as buttons, which uses the active class to show that something is selected.

This change adds $hover-background and $hover-border as options to the button-outline-variant mixin, and gives them default values half-way between than the $color and white.

I think this is related to #26804

Current state

old-bad

With change

new-nice

Closes #26804

Preview: https://deploy-preview-28999--twbs-bootstrap.netlify.com/docs/5.1/components/buttons/#outline-buttons

@mermop mermop requested a review from a team as a code owner July 9, 2019 01:21
@MartijnCuppens
Copy link
Member

Personally I think we better darken the hover color instead of making them completely dark.

@XhmikosR
Copy link
Member

XhmikosR commented Aug 9, 2019

I agree, this doesn't feel natural to me.

@MartijnCuppens
Copy link
Member

I just pushed some changes which fixed the hover color.

MartijnCuppens
MartijnCuppens previously approved these changes Apr 27, 2020
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.

I like this

@XhmikosR XhmikosR requested a review from mdo April 27, 2020 17:50
@XhmikosR XhmikosR added this to Inbox in v5 via automation May 13, 2020
@XhmikosR
Copy link
Member

@mdo: WDYT?

@XhmikosR
Copy link
Member

Rebased, I need a second look please @MartijnCuppens.

@mermop please do not overwrite the rebase by accident 🙂

@MartijnCuppens
Copy link
Member

Looks good

@XhmikosR
Copy link
Member

OK, then we need @mdo @ysds @ffoodd to chime in :)

@ffoodd
Copy link
Member

ffoodd commented May 15, 2020

Already commented on this, I think arguments naming could be more consistent with surroundings.

Apart from that, it's good 🙂

@XhmikosR
Copy link
Member

Feel free to rename any args, we can edit the files ourselves unless the user has explicitly turned this off 🙂

@ffoodd
Copy link
Member

ffoodd commented May 15, 2020

Mixin arguments are now consistent —in both naming and order— with previous mixin.

@mdo mdo removed this from Inbox in v5 Jun 1, 2020
@mdo
Copy link
Member

mdo commented Jun 1, 2020

Not in favor of launching v5 alphas with this just yet. It's a pretty strong departure from our current styles and I feel a decent amount of outline buttons roll with this approach. It makes the active state feel out of place somehow.

@mdo mdo changed the base branch from master to main June 16, 2020 20:18
@patrickhlauke
Copy link
Member

Looking at https://deploy-preview-28999--twbs-bootstrap.netlify.app/docs/5.0/forms/checks/#outlined-styles i could well go for that (and yes that would solve #31149). not 100% convinced it looks natural for https://deploy-preview-28999--twbs-bootstrap.netlify.app/docs/5.0/components/buttons/#outline-buttons though...maybe do a halfway implementation where this only targets the checkbox/radio buttons (and https://deploy-preview-28999--twbs-bootstrap.netlify.app/docs/5.0/components/buttons/#toggle-states outline ARIA toggles) scenario, but not the regular "one shot" buttons? or does that get too fiddly?

@mdo
Copy link
Member

mdo commented Jan 14, 2021

I’m sorry for dropping the ball on the pings here. With the beta phase, do folks consider this a breaking change?

@ffoodd
Copy link
Member

ffoodd commented Jan 14, 2021

@mdo Yes. If someone used the mixin with named arguments (eg @include button-outline-variant($color-hover: #111), it'll break.

@wcarss
Copy link

wcarss commented Apr 18, 2021

Hi, not sure if this is relevant and not trying to derail -- I can post a separate issue but some similar ones (for mobile, etc.) got marked as dupes, and everything I could find (e.g. #31149) leads to this thread.

I'm on osx desktop chrome 89, and when I click on a toggle state button and mouse away, the focus state is retained, and the current colour choices (in 4.5, 4.6, and 5.0-beta3) for focused/hovered/active make it very hard to tell what the state of the most-recently-clicked button is, until the user clicks elsewhere on the page to dispel the focus-state. Here's a gif from this PR's preview showing a full state-cycle: https://deploy-preview-28999--twbs-bootstrap.netlify.app/docs/5.0/components/buttons/#toggle-states:

You can see after clicking and mousing off, focus is retained and the state looks roughly "active", but much worse is that when toggling off the change is very slight -- it still pretty much looks active, and you have to click somewhere else to know what the state really is.

I ran into this in the wild on arewefastyet.rs today, which starts with some buttons activated that you might naturally turn off -- what is the state of the # of cores component below? With no siblings active, even with other identical active components nearby, it's really tough to tell none were selected until the off-element click.

I actually found their github and cloned their repo just to try to fix this, thinking it was some missing/broken mouseout handler issue, before realizing it's ...just bootstrap's default behaviour+appearance.

If this PR isn't the right place for this or folks don't agree that what I've posted an issue, I'd be happy to re-file/discuss somewhere else.

@patrickhlauke
Copy link
Member

Would be good to revisit this, as it's been a long-standing issue/problem we've had for aeons now.

@shivamCode0
Copy link

When I clicked a button on the example, it works but it flashes the color because the :hover is not opaque enough.

@GeoSot
Copy link
Member

GeoSot commented Aug 29, 2021

@twbs/css-review any news on this?

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.

I'm still up for this, however this is for a minor release and should be highlighted in the changelog as a potential BC.

@XhmikosR XhmikosR added this to In progress in v5.2.0 via automation Dec 17, 2021
@XhmikosR XhmikosR removed the on-hold label Dec 17, 2021
@XhmikosR
Copy link
Member

XhmikosR commented Dec 17, 2021

My only concern is if we should land this in v5.2.0 (a minor version) or not. It is a potential breaking change, isn't it? If so, is just mentioning in the release notes enough/are we still following semver?

@XhmikosR XhmikosR removed this from In progress in v5.2.0 Dec 17, 2021
@XhmikosR XhmikosR changed the title Differentiated hover state for outline buttons Differentiate hover state for outline buttons Dec 17, 2021
@XhmikosR XhmikosR added this to In progress in v5.2.0 via automation Dec 18, 2021
@XhmikosR XhmikosR added this to In progress in v5.3.0 via automation Dec 18, 2021
@XhmikosR XhmikosR added this to Inbox in v6.0.0 via automation Dec 18, 2021
@mdo mdo removed this from In progress in v5.2.0 Feb 10, 2022
@patrickhlauke
Copy link
Member

forgot about this PR, and separately did some work here #37026 ... thoughts?

@mdo
Copy link
Member

mdo commented Sep 4, 2022

Superseded by #37026.

@mdo mdo closed this Sep 4, 2022
@mermop mermop deleted the patch-2 branch December 16, 2022 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v6.0.0
Inbox
v5.3.0
In progress
Development

Successfully merging this pull request may close these issues.

Bad UX for Checkbox Button on mobile browser
9 participants