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

Remove stroke from CSS rules that colorize plugin icons #13719

Merged
merged 2 commits into from
Feb 8, 2019

Conversation

avillegasn
Copy link
Contributor

@avillegasn avillegasn commented Feb 7, 2019

Description

There are some CSS rules that add stroke to colorize plugin icons. That makes plugin icons look weird as you can see here:

with stroke

After removing the stroke from the CSS rules, plugin icons look great:

without stroke

This happens with both SVG files and dashicons when filling the icon argument of wp.plugins.registerPlugin.

How has this been tested?

I just registered a plugin with the following:

wp.plugins.registerPlugin( 'my-plugin', {
	icon: 'translation', // the dashicon
	render: Component,
} );

Types of changes

My code just removes the stroke property in three CSS rules.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@youknowriad youknowriad requested a review from jasmussen February 7, 2019 09:02
@gziolo gziolo added Needs Design Feedback Needs general design feedback. General Interface Parts of the UI which don't fall neatly under other labels. labels Feb 7, 2019
@jasmussen
Copy link
Contributor

Thank you for the pull request!

This is an interesting one. The reason we set those properties is to enforce consistency for plugin icons, specifically to ensure contrast when a button is toggled on or off. I know this is done a little heavy-handedly, even with !importants and everything.

But this PR does surface a challenge this brings to the SVG plugin developers can use for these slots. Specifically, if the icon uses both strokes AND fills to render the icon, it might cause situations where one of the two has the wrong color.

@avillegasn is there any chance you can share the specific SVG file (you can paste it here inline) that's causing trouble? I suspect that there's a white stroke around the base fill, which gets colorized a dark gray in the editor.

I'm tempted to suggest the solution here isn't to change these styles in the editor, but rather create documentation for how to create these icons. If I'm right that there's a white stroke, the real solution is to flatten the SVG to one compound vector shape.

Removing just the stroke property is only solving the problem for an icon that uses dark fills and light strokes, and then the inverse problem, an icon that uses light fills and dark strokes, will stop working.

@avillegasn
Copy link
Contributor Author

Thanks for the quick response!

@avillegasn is there any chance you can share the specific SVG file (you can paste it here inline) that's causing trouble?

I just used the translation dashicon (that apparently you can find here). Take a look at the example I included:

wp.plugins.registerPlugin( 'my-plugin', {
	icon: 'translation', // the dashicon
	render: Component,
} );

I suspect that there's a white stroke around the base fill, which gets colorized a dark gray in the editor.

Does this mean WordPress dashicons are not properly created and need to be fixed?

@jasmussen
Copy link
Contributor

Oh goodness yes, you gave that example, sorry for missing that. Let me investigate! From a casual glance it looks like that SVG is alright and I was wrong in my assessment. But yes, if I had been right, we'd need to fix that in Dashicons.

Let me take a deeper look and return, thank you for the response! 🌟

@avillegasn
Copy link
Contributor Author

I think we need to explicitly state that the SVG file doesn't have a stroke. For instance, adding the property stroke-width="0" to the path elements in the SVG file seems to fix the issue.

@jasmussen
Copy link
Contributor

I think we need to explicitly state that the SVG file doesn't have a stroke. For instance, adding the property stroke-width="0" to the path elements in the SVG file seems to fix the issue.

Excellent suggestion. Here's a codepen that illustrates the problem.

https://codepen.io/joen/pen/JxOzwP?editors=1100

Try to comment out the line that says stroke-width: 0; to reproduce the issue seen in this pull request.

But if we, as you suggest, add this property back, not only does it fix the problem but it doesn't negatively affect icons that consist ONLY of strokes, because these icons are likely to have their stroke-widths defined as inline elements styles regardless, so they will override the value.

@jasmussen
Copy link
Contributor

@avillegasn can you restore the stroke values and add the stroke-width fix? It should look like this:

.components-icon-button:not(.is-toggled) svg,
.components-icon-button:not(.is-toggled) svg * {
	stroke: $dark-gray-500 !important;	
	fill: $dark-gray-500 !important;
	stroke-width: 0; // !important is omitted here, so stroke-only icons can override easily.
}

Also add to the subsequent toggled states. Then let's get this in, and thank you for the fix!

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Very nice, thank you!

@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels. Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants