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

Add "outline-none" and "shadow-outline" utilities #491

Merged
merged 7 commits into from Jun 20, 2018
Merged

Conversation

reinink
Copy link
Member

@reinink reinink commented Jun 14, 2018

This is an alternative to #490, and implements what I suggested in #56 (comment).

  1. This adds new outline module, which includes a single outline-none utility.
  2. This adds a new default shadow utility, called shadow-outline. Since box-shadows are more useful than outlines in CSS (mostly because they respect border-radius), it makes more sense to use them than real CSS outlines.
  3. This enables the focus variant for shadows and outlines by default.
  4. This removes the default tabindex="-1" outline style from Preflight.

Closes #56
Closes #385
Closes #427

@joshmanders
Copy link
Contributor

Ship it!

@DCzajkowski
Copy link

What do you think about naming them outline-none and outline-shadow respectively? It'd be more consistent in my opinion.

@adamwathan
Copy link
Member

What do you think about naming them outline-none and outline-shadow respectively? It'd be more consistent in my opinion.

All box shadow utilities have to start with shadow-; you can only specify the modifier name in the config and I think it would be misleading to call it outline-shadow because it sounds like it's setting the outline CSS property when it's actually setting box-shadow.


export default function() {
return defineClasses({
'no-outline': { outline: 'none' },
Copy link
Member

Choose a reason for hiding this comment

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

Bootstrap uses outline: 0 for theirs, not sure if it really matters but when there are multiple ways to do things I think it's a good idea to use precedent from a huge project with tons of users. Saves a couple bits in the compiled CSS too I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, okay, will update.

@reinink reinink mentioned this pull request Jun 15, 2018
@DCzajkowski
Copy link

@adamwathan Yeah, true. It would be nice to have all outline-related classes with the outline- prefix, but there is no good way of doing that. But it should be outline-none for consistency.
There is shadow-none, appearance-none, select-none, resize-none, pointer-events-none, border-none, leading-none, pin-none, float-none and only one no-underline.

@adamwathan
Copy link
Member

@DCzajkowski Yeah I don't really mind calling it outline-none or outline-0 (which is more accurate since that's how it's actually implemented). I prefer no-outline personally because I don't have to remember "is it none or 0?" (I mess this up constantly with rounded-none already) but I can appreciate that it feels more consistent, and sets us up better down the road if we ever decided it was valuable to add a real customizable outline module. The consistency argument is a little tough still though, because it would be very hard to convince me to change no-underline to decoration-none for example, heh.

@reinink What do you think about removing the tabindex outline styles from preflight mentioned in #427 as part of this PR? I figure it's more flexible to make that behavior opt-in by writing <div class="no-outline" tabindex="-1"> than to force it on everyone.

@adamwathan
Copy link
Member

Also as part of this PR, we might want to consider enabling the focus variant for the shadows module by default.

Also wondering, if you apply outline: none to a non-focus selector, does that trump the user agent focus styles in all browsers? I know it does in Chrome for sure, but might we need to actually disable outline on focus instead of with no pseudo-class?

.no-outline {
  outline: none;
}

// vs.

.focus\:no-outline:focus {
  outline: none;
}

@reinink
Copy link
Member Author

reinink commented Jun 20, 2018

Also as part of this PR, we might want to consider enabling the focus variant for the shadows module by default.

Yup, totally.

I know it does in Chrome for sure, but might we need to actually disable outline on focus instead of with no pseudo-class?

I've never done it that way before.

@reinink
Copy link
Member Author

reinink commented Jun 20, 2018

Okay, updated:

  1. Renamed "no-outline" to "outline-none" instead
  2. Enabled focus variants for shadows by default
  3. Enabled focus variants for outlines by default
  4. Removed default tabindex="-1" outline style in Preflight

@reinink reinink changed the title Add "no-outline" and "shadow-outline" utilities Add "outline-none" and "shadow-outline" utilities Jun 20, 2018
@adamwathan adamwathan merged commit 1fc8a85 into master Jun 20, 2018
@adamwathan adamwathan deleted the no-outline branch May 13, 2019 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants