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

Make .close hover focus styles less specific #27785

Closed
wants to merge 1 commit into from

Conversation

pat270
Copy link
Contributor

@pat270 pat270 commented Dec 5, 2018

This pull removes the &:not(:disabled):not(.disabled) selector on the .close component and styles :disabled/.disabled directly because the :not pseudo classes are too hard to overwrite (breaks for people that extended .close based on the old version).

Also added variables:

$close-opacity
$close-hover-color
$close-hover-opacity
$close-disabled-opacity

Fixes #27784

@pat270 pat270 requested a review from a team as a code owner December 5, 2018 23:29
@XhmikosR XhmikosR changed the title Make .close hover focus styles less specific (#27784) Make .close hover focus styles less specific Dec 6, 2018
scss/_close.scss Outdated Show resolved Hide resolved
@mdo
Copy link
Member

mdo commented Dec 7, 2018

We moved to this approach awhile ago across most of the project to simplify our the amount of code to properly disable things. If we change it here, I fear we need to change it everywhere. Are there other places we need to revisit as well?

@pat270
Copy link
Contributor Author

pat270 commented Dec 7, 2018

Changing it across the board is my preference, but might cause some unintended consequences for any Bootstrap themes in the wild.

&:not(:disabled):not(.disabled) is used in a handful of places buttons, close, navbar and pagination, generally for the cursor property. These existed in 4.0.0 and we were able to account for these. The changes to .close for:

  &:not(:disabled):not(.disabled) {
	    @include hover-focus {
	      opacity: .75;
	    }

came too late unfortunately. What if we kept:

  &:not(:disabled):not(.disabled) {
    // Opinionated: add "hand" cursor to non-disabled .close elements
    cursor: pointer;
  }

and moved the hover-focus opacity outside of that selector?

@mdo
Copy link
Member

mdo commented Dec 16, 2018

That solution could work perhaps, but then we're still adding extra selectors. Going to have to review this more in-depth.

@pat270
Copy link
Contributor Author

pat270 commented Dec 18, 2018

@mdo if you prefer to change it everywhere, I can resend with changes everywhere.

@XhmikosR XhmikosR changed the base branch from v4-dev to master February 19, 2019 14:08
@XhmikosR XhmikosR requested a review from a team May 10, 2019 07:08
@ysds
Copy link
Member

ysds commented May 10, 2019

I do not know why .close supports <a>. <button> is enough for accessibility.

In general you should only use an anchor for navigation using a proper URL.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#Accessibility_concerns

If we drop support of <a>, its CSS will be more simple.

@patrickhlauke
Copy link
Member

I do not know why .close supports <a>

Because sadly, many authors (either directly, or indirectly because their tooling does it) like to misuse <a> for button-style controls...

@ysds
Copy link
Member

ysds commented May 10, 2019

@patrickhlauke OK I understand.

@pat270 could you fix the conflict ?

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.

Bootstrap 4.1.2 .close hover focus styles are too specific
6 participants