Skip to content

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
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 ffoodd requested a review from MartijnCuppens May 15, 2020 13:59
@ffoodd
Copy link
Member

ffoodd commented May 15, 2020

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

@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?

@XhmikosR
Copy link
Member

@mdo what's your take on this?

@XhmikosR XhmikosR dismissed MartijnCuppens’s stale review November 25, 2020 08:03

See Mark's comment

@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 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 changed the title Differentiated hover state for outline buttons Differentiate hover state for outline buttons Dec 17, 2021
@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

Status: Inbox

Development

Successfully merging this pull request may close these issues.

Bad UX for Checkbox Button on mobile browser

9 participants