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

Added !important to sr-only and sr-only-focusable SASS mixins #28701

Closed
wants to merge 3 commits into from
Closed

Added !important to sr-only and sr-only-focusable SASS mixins #28701

wants to merge 3 commits into from

Conversation

Jakobud
Copy link

@Jakobud Jakobud commented Apr 28, 2019

Closes #28691

@Jakobud Jakobud requested a review from a team as a code owner April 28, 2019 06:16
@Jakobud
Copy link
Author

Jakobud commented Apr 28, 2019

This is regarding #28691

patrickhlauke
patrickhlauke previously approved these changes Apr 28, 2019
@ysds
Copy link
Member

ysds commented Apr 28, 2019

@patrickhlauke I added comment in #28691. Could you re-review the necessity need to set "! Important" for all properties?

@patrickhlauke
Copy link
Member

@ysds i really don't see the harm of just having everything !important regardless of explicit need. to me, makes it easier than having to potentially miss out/test permutations of what does/doesn't need to be set...
so whatever, retracting my review for now...

@MartijnCuppens
Copy link
Member

Hmm, this looks like a nothing can go wrong here PR, but @ysds' seems to be right here. Adding !important to everything can lead to some unexpected results. In fact even without !important we already have an issue: https://codepen.io/MartijnCuppens/pen/Pgrpwe

I think we should have a look at which properties are still necessary and then rethink about which should be !important before maybe introducing more problems.

I also just noticed we did a way shorter notation for the button groups, maybe that's more what we should look into:

input[type="radio"],
input[type="checkbox"] {
position: absolute;
clip: rect(0, 0, 0, 0);
pointer-events: none;
}

@Jakobud
Copy link
Author

Jakobud commented May 1, 2019

I'm not sure I'm seeing the issue in that codepen. The sr-only button appears only when focused, which appears to be functioning correctly...

EDIT: Ah I see now the button sizing or padding is not correct.

@Jakobud
Copy link
Author

Jakobud commented May 1, 2019

Maybe the solution on sr-only-focusable would be to set values to use inherit so that they inherit their values (padding, margin) to what they should be according to their other classes when it's focused...

@XhmikosR XhmikosR dismissed patrickhlauke’s stale review May 2, 2019 07:41

Needs further investigation

@Jakobud Jakobud closed this May 3, 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.

.sr-only should use !important
5 participants