Use currentcolor as border-color for outline button style

[chrisvanpatten]

This updates the "Outline" button style to use currentcolor—aka the current text color—as the border color, per a @mtias comment in Slack (I'm considering that comment "design feedback" but I'm also happy to request additional feedback if desired.)

To override a potential inline style attribute, an !important is unfortunately required. I think this is acceptable in this case but I'm also open to other ideas!

Screenshot

[ZebulanStanphill]

Why can't the background color option still be used?

[chrisvanpatten]

@ZebulanStanphill The outline style is intended to have a transparent background, so content behind it (such as an image) peeks through. If you want to use a background, you can use the default/normal button style.

[ZebulanStanphill]

@chrisvanpatten But what if I want to have a button with an outline and a background? If you want the background to be transparent, why not just clear the background color in the inspector? It seems weirder to have the background color option still visible but do nothing.

[chrisvanpatten]

@ZebulanStanphill If you need a different style, you can register a new block style that provides whatever capabilities you need.

There is also discussion of changing the code to hide the background color (or other options, as appropriate) based on the block style, but this is focussed on the CSS side, not the interface side.

As to why we are putting this limit in place, it's fundamentally a subjective design decision, but with the ability to register additional styles users should have flexibility to build out whatever visuals they need.

[jasmussen]

Thank you for this PR. This is cool, and it seems to work well, both for the placeholder, the style preview, and the color thing itself:

There's one thing we need to figure out though, and you're already discussing it. In fact I agree equally with both of you:

Why can't the background color option still be used?

The outline style is intended to have a transparent background, so content behind it (such as an image) peeks through. If you want to use a background, you can use the default/normal button style.

The outline style is exactly as noted, intended to be transparent so content behind can peek through. You may already be bored to death of this, but imagine a Cover image dimmed with fixed background, then an outline button on top.

However then there's this:

Basically, the background option in the sidebar is still present, but in this style it simply doesn't do anything. This is confusing, and although the argument about this variation being transparent is right, I don't think a layperson would grok this.

So it seems there are two paths ahead:

1. Let the background color work. Okay, it goes against the spirit of the style to have a background color, but at least it will be functional. Maybe you can make it semi-transparent to sort of embrace the transparent nature?

2. Hide the background color option when this style is picked. Not sure how easy or feasible this is.

What do you think? 1 seems like the easiest fix.

[jasmussen]

Okay I don't know what's going on with Github right now, but I've saved my comments as text files. They seem to show up some times and not others.

I apologize if I've spammed you all with emails for trying to submit multiple times, but this outage has been worse than most!

[ajitbohra]

@jasmussen seems like an issue on GitHub can see multiple comments here and on other issues/PRs.

[chrisvanpatten]

GitHub is definitely super laggy today, but I did fix this up per your feedback and our Slack discussion @jasmussen. Once GitHub calms down it's ready for re-review!

[chrisvanpatten]

Travis is passing so we're ready for review! 🚀

[mtias]

Actually, I think if you have the "outline" style the background color should be reset and the options removed from the inspector.

[ZebulanStanphill]

@mtias Personally, I disagree. I think hiding options in the inspector based on the selected style variation is kind of weird and confusing.

[chrisvanpatten]

I'm happy either way although right now I don't think we can get style variations in the Inspector to conditionally show/hide fields (since they were intended to just apply simple class names).

[youknowriad]

@chrisvanpatten Providing the current style as a prop to edit functions is something I was planning to work on soon.

[mtias]

I think hiding options in the inspector based on the selected style variation is kind of weird and confusing.

Why? Some options might only be relevant to certain styles and confusing with others.

[chrisvanpatten]

I don't think the UI experience is made less clear but I think the dev experience and purpose of block style variations would be a bit muddled.

Style variations were originally intended solely to apply a classname and basically be totally agnostic/ignorant about the controls that were available and the attributes that were set. It applies the class name and walks away.

And the reverse was also true: blocks themselves would not need to worry about block style variations because, ultimately, a theme could register any number of variations with infinite class names and the block wouldn't need to be updated to know about them.

Allowing a block to respond to specific style variations changes that contract (or I guess changes the intentional lack of a contract). Before, blocks and style variations were effectively ignorant of each other; allowing a block to apply special privileges and behaviors to certain styles is very much in opposition to that.

I'm not saying that's necessarily bad, just that it changes the initial parameters.

Things to consider further: would the block continue to work smoothly if a style variation is unregistered? Should the style variation attribute be passed down into the save function as well for render-time changes?

[chrisvanpatten]

In the interest of incremental progress, maybe we can land this iteration — using currentcolor for the border color and allowing custom backgrounds — and iterate on the block style/block controls relationship in a future PR?

What do you think @mtias @jasmussen?

[jasmussen]

I'll defer to the others, because I'm good with that.

[youknowriad]

Changes look good to me 👍

[davewhitley]

Not sure what was decided on, but I think there's another option. I agree it's a bad experience when the background settings don't appear to be doing anything, but it's also not great when settings only appear based on certain selected styles.

Before reading this PR, I fully expected the background setting to apply to the border of the button (when I have the outlined style applied).

So instead of this:

it would be this

---

You would change the border color of the outlined button by changing the background color. I think both of the issues presented earlier would be solved with this method.

Yes, technically "background color" is not the same as "border color" but I wouldn't assume it would matter to users. Background color is just the color of the button, whether it be an outlined button or a solid button. As a user I still consider the outline the "background" of the button — it's just a background with a border and a transparent fill.

Text color would only affect the text.

Hope this helps!

[shaunandrews]

Instead of "Background Color" maybe "Shape Color" or "Button Color"?

[webmandesign]

Hi guys, late, but I think using currentColor is redundant.
So we can further optimize with not setting any border color value (and remove the hover/focus/active state border color styles too). Here is an example: http://jsfiddle.net/kerxochv/1/

Would this do? Should I create an issue ticket and then submit a fixing PR? Or is there any particular reason why we need to use currentColor that I'm missing here?

[aduth]

@webmandesign You may be right, specifically by the standard behavior of an unset border-color defaulting to the element's color value:

The border color properties specify the color of a box's border.

Initial: | the value of the 'color' property

https://www.w3.org/TR/CSS2/box.html#border-color-properties

I guess if we can assume "the value of the 'color' property" and currentColor are interchangeable, there's no need for it.

[chrisvanpatten]

My only concern is whether we need the extra specificity for theme compat, but if someone can test and verify it works without currentcolor with the twenty{$year} themes, I see no reason to keep it in!

[webmandesign]

@aduth Yes, those are interchangeable for sure. I have a lot of experience with border colors in my themes ;)

@chrisvanpatten This should cause no issue. We already have an override with:
.wp-block-button.is-style-outline .wp-block-button__link { border: 2px solid; } That resets all border styles, including color.

I'll go ahead an create an issue ticket about currentColor in border declaration and I'll also submit a PR fixing all the issues I've found.

[webmandesign]

FYI, #12328