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

Fix colors of plugin icons. #7470

Merged
merged 3 commits into from Jun 27, 2018

Conversation

@jasmussen
Contributor

jasmussen commented Jun 22, 2018

This PR does two things:

  1. It forcefully colorizes any SVG icon used in the toolbar or More menu. This allows us to be sure the contrast is present, and that we can show a toggled and untoggled color.

  2. It adds margin to plugin icons, this fixes #7464.

Yes. I'm using !important here. I know you're not supposed to do that. But aside from being the only way we can ensure the colors, I feel like this is actually a valid usecase for the modifier.

Screenshots:

button color

screen shot 2018-06-22 at 12 48 51

Fix colors of plugin icons.
This PR does two things:

1. It forcefully colorizes any SVG icon used in the toolbar or More menu. This allows us to be sure the contrast is present, and that we can show a toggled and untoggled color.

2. It adds margin to plugin icons, this fixes #7464.

@jasmussen jasmussen added this to the 3.2 milestone Jun 22, 2018

@jasmussen jasmussen self-assigned this Jun 22, 2018

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Jun 22, 2018

g,
path {
stroke: $dark-gray-500 !important;
fill: $dark-gray-500 !important;

This comment has been minimized.

@youknowriad

youknowriad Jun 22, 2018

Contributor

Plugins can still override using !important. I'm not certain we should/can enforce that personally.

This comment has been minimized.

@jasmussen

jasmussen Jun 22, 2018

Contributor

The benefit of us doing this as strongly as we can, is that the plugin developer doesn't have to do anything on order to get the icon to work on a dark background.

While I don't feel super strongly here, I think we should start out opinionated, and then we can consider pulling back if a valid use case presents itself.

// Forcefully colorize plugin icons to ensure contrast and cohesion.
svg,
g,
path {

This comment has been minimized.

@aduth

aduth Jun 25, 2018

Member

The choice of elements to target here seems ad-hoc and incomplete, and I worry it could result in some abominations of icons where only parts are recolored (and not other parts such as circle, ellipse, line, etc).

Example: https://codepen.io/aduth/pen/qKyxXJ

Would a wildcard * descendent selector be better?

This comment has been minimized.

@mtias

mtias Jun 25, 2018

Contributor

Yes, I think we should not be forcing anything here. cc @jasmussen

This comment has been minimized.

@jasmussen

jasmussen Jun 25, 2018

Contributor

I dislike using !important as much as the next one. But most SVGs use inline styles for strokes and colors, and if we don't use !Iimportant, it means that the icon will have whatever color the plugin author provides, including when the icon is toggled, where it should be white on the gray toggle background. In other words, if an SVG has fill: green; as an inline style, it will be green in every single context, hover, toggled, active etc.

This comment has been minimized.

@jasmussen

jasmussen Jun 25, 2018

Contributor

As an example:

screen shot 2018-06-25 at 15 47 37

That's when I remove the !important from the is-toggled CSS class.

This comment has been minimized.

@youknowriad

youknowriad Jun 25, 2018

Contributor

@jasmussen I thought I had styles for that in the plugin itself (white path when toggled :P )

This comment has been minimized.

@youknowriad

youknowriad Jun 25, 2018

Contributor

As a "plugin developer" I think I agree with Gutenberg forcing a white path when toggled but I also think it shouldn't force a gray color when untoggled. Plugins should only care about providing the right colors for the default state (untoggled).

This comment has been minimized.

@jasmussen

jasmussen Jun 25, 2018

Contributor

I'd be okay with forcing colors when toggled.

This comment has been minimized.

@aduth

aduth Jun 25, 2018

Member

To clarify, I have no issue with forcing the color, just that there are more colorable SVG elements than just svg, g, and path, and we're not overriding them consistently.

This comment has been minimized.

@mtias

mtias Jul 5, 2018

Contributor

@jasmussen sorry I missed the follow ups here. I agree with forcing on toggle state, but I think e should leave the colors the plugin chose for the normal state.

This comment has been minimized.

@jasmussen

jasmussen Jul 5, 2018

Contributor

Cool. That's what we ended up doing.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 26, 2018

Okay, took a new stab at addressing the feedback. Here's how it looks now:

icons

I'm targetting svg, svg * as the selectors for coloring. What happens now, as you can see starting 66de82b#diff-cbe9c99c74fcdc63180d60c4535ed4eeR8 is this:

  • We colorize SVG icons provided by the user. Not forcefully, but we do this so if a user downloads a material icon from https://material.io/icons/ and doesn't do anything to it, at least it's gray — those icons come without inline styles. But we don't do it forcefully, so if like in this example one icon is green and the other is purple, the plugin author can do that.
  • We forcefully colorize the toggled state. This ensures the icon can be seen when toggled.
  • We forcefully colorize the hover state, this ensure that there * is * a hover state — without this, the base style would still shine through. In addition to this, it is necessary in order to be able to hover a toggled button.

Thoughts?

@tofumatt

Wondering about Firefox SVG stuff… maybe I'm forgetting what works and what doesn't, but I recall having issues colouring SVGs back in the day...

svg,
svg * {
stroke: $dark-gray-500;
fill: $dark-gray-500;

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

Has all of this been tested in Firefox? I recall having issues back when we wanted to CSS colour our SVGs on the Firefox Add-ons site making it tough to use CSS styles for SVGs.

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

Update: hover works in Firefox but the Dropit plugin shows colours unless I force !important on more SVG selectors:

2018-06-26 17 39 44

This comment has been minimized.

@jasmussen

jasmussen Jun 27, 2018

Contributor

Per discussion in #7470 (comment), this is intended behavior.

That is: if an SVG does not provide inline styles, our gray color will apply for the pinned non-toggled non-hovered icon.

Regardless of inline styles, the colors will be enforced on hover and when toggled.

@tofumatt

Looks good to me, just some minor nits.

But it works in Firefox for me. Nice idea!

.components-menu-items__item-icon {
margin-right: 5px;
.components-menu-items__item-icon,
> span > svg { // Also target plugin icons that can have arbitrary classes.

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

This is a weird spot for a comment, can it go above the selector instead?

It doesn't need the "also" either (the comma implies that).

// Target plugin icons that can have arbitrary classes by using an aggressive selector. should do the trick!

@@ -27,6 +28,19 @@
&:focus:not(:disabled):not([aria-disabled="true"]) {
@include menu-style__focus;
}
// Colorize plugin icons to ensure contrast and cohesion, but allow plugin devs to override.

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

Let's say "developers" 😄

@@ -4,4 +4,24 @@
.components-icon-button {
margin-left: 4px;
}
// Colorize plugin icons to ensure contrast and cohesion, but allow plugin devs to override.

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

This is repeated, which seems strange. But if it's staying we should at least say "developers".

fill: $dark-gray-500;
}
// Forcefully colorize hover & toggled plugin icon states to ensure legibility and consistency.

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

I guess the ampersand is fine… but why not just write "and" 😄

margin-right: 5px;
.components-menu-items__item-icon,
> span > svg { // Also target plugin icons that can have arbitrary classes.
margin-right: 4px;

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

Oh, this is covered by our weird RTL transform thing, right? I forget how it works. I think this is okay, but I forget.

This comment has been minimized.

@jasmussen

jasmussen Jun 27, 2018

Contributor

CC: @yoavf it should, right?

This comment has been minimized.

@yoavf

yoavf Jun 27, 2018

Contributor

Yes, RTLCSS will take care of that in RTL mode.

This comment has been minimized.

@jasmussen

jasmussen Jun 27, 2018

Contributor

Thank you!

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 27, 2018

Love your feedback! All the typos have been fixed, and I'll merge when green, noting that if there are disagreements on the forcefullness, we can always revisit that.

@jasmussen jasmussen merged commit c060981 into master Jun 27, 2018

2 checks passed

codecov/project 46.77% (-0.04%) compared to 58c96ee
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the fix/plugin-icons branch Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment