More specific CSS selector for SVG icons. #5352

afercia · 2018-03-02T11:39:21Z

Description

This PR tries to use a more scoped CSS selector for the SVG icons.

targets only the editor area, excluding the core admin menu, toolbar, and footer
targets only the SVG icons with a dashicon class

Solves a problem when plugins use their own SVG icons and the previous selector is too aggressive:
body.gutenberg-editor-page svg { ... }

is a bit overqualified and targets any SVG in the whole page, even SVG that aren't actual icons. Plugins may also use SVG images, logos, etc..

How Has This Been Tested?

I've manually tested the Gutenberg icons still look good.

Note: about the new block mover icons, I've noticed they inherit a slightly different color /cc @jasmussen

Fixes #5302

afercia · 2018-03-05T14:14:39Z

Hm, I don't think the block mover icons should have a tabIndex="-1". Added the dashicon css class for consistent styling.

aduth · 2018-03-06T02:06:19Z

editor/components/block-mover/index.js

@@ -24,13 +24,13 @@ import { selectBlock } from '../../store/actions';
 * Module constants
 */
 const upArrow = (
-	<svg tabIndex="-1" width="18" height="18" xmlns="http://www.w3.org/2000/svg" aria-hidden role="img" focusable="false">
+	<svg className="dashicon" width="18" height="18" xmlns="http://www.w3.org/2000/svg" aria-hidden role="img" focusable="false">


Asked about the intent of these icons at #5263 (comment), but I generally would discourage pretending to inherit styles from a separate component.

I'd agree best would be to have them integrated into Dashicon

aduth · 2018-03-06T02:06:57Z

edit-post/assets/stylesheets/main.scss

@@ -104,6 +99,11 @@ body.gutenberg-editor-page {
 		font-size: $default-font-size;
 		color: $dark-gray-500;
 	}
+
+	svg.dashicon {


If these styles are targeted at Dashicon, seems sensible that they be added to a components/dashicon/style.scss, also because they don't seem specific to Gutenberg.

As far as I know, components/dashicon/index.js is a copy from the Dashicons repo, how to import a new components/dashicon/style.scss in this file if then it gets overridden with any update from the Dashicons repo?

It's a tough one. I think long-term this approach of copy-paste from the Dashicons repository is not very sustainable. Ideally we'd just import from a Dashicons module. Where that repository lives, I'm not sure (it could even stay where it's at, or be merged to here). Then, we'd change components/dashicon/index.js to...

/** * WordPress Dependencies */ import Dashicon from '@wordpress/dashicon'; /** * Internal dependencies */ import './style.scss'; export default Dashicon;

It's an open question though whether those styles should be bundled with the shared component itself. Alternatively, we could assign the styles as an inline style attribute, though of course these are much harder to override in their high specificity.

It's possible (though not implemented in Gutenberg anywhere) to implement the styles as a stylesheet from the Dashicon module and import into the project as well.

I understand all the concerns 🙁 However, this issue is specifically about changing the CSS selector used for this rule. The placement of the rule is unrelated. Can we split the two issues and move on?

In resolving one issue, you've introduced two others which did not exist previously (styles applied to a component outside its stylesheet, ad hoc inheriting styles from a component elsewhere in the codebase). Arguably the style placement code conventions are less important than the user-impacting ones fixed here, but all the same I'm not inclined to give blessing to the accumulation of technical debt.

@aduth: the styles were already in this file. I've just added .dashicon 🙂 I haven't moved anything nor introduce a new issue. The issue you're pointing out is a pre-existing issue and unrelated.

I've just added .dashicon 🙂

This is the issue.

It really doesn't change the original issue. The previous CSS selector was already styling the dashicon SVGs.

jasmussen · 2018-03-06T18:55:13Z

I'll take a look at this tomorrow.

afercia · 2018-03-06T21:24:57Z

By the way, seems svg icons are styled also from other files, see

gutenberg/components/toolbar/style.scss

Line 63 in e7c95d8

& > svg {

so this styles placement is definitely a pre-existing issue.

jasmussen · 2018-03-07T10:39:07Z

As part of #5400, I'm going to be redesigning the move icons, up/down, and I'll be doing some refactoring of that area specifically. The reason those icons were inline instead of from Dashicons was that they were smaller (18px) than the Dashicon base grid (20px). So the SVG changes in this PR are likely going to be moot as I have to revisit that area anyway.

With regards to the CSS changes, here's a bit of context. The PR scopes these two styles:

fill: currentColor;	
outline: none;

Both of those are indeed necessary for icon SVGs, all of the icons, not just Dashicons. But they are, as correctly deduced, not necessary for every SVG. For example if a plugin block decides to use an SVG as an illustration, the fill property shouldn't necessarily remove all color from it. So the intent of the PR is definitely sensible.

But I'm not sure what the best approach is. I can definitely add an upstream PR to the Dashicons repo that outputs a helper style.scss file to the component directory, this is trivial to do. This style could contain, in its entirety:

.dashicon {
	fill: currentColor;
	outline: none;
}

This might address most of the issues. But what happens when plugins start adding SVG for icon usage, for example when sidebar API plugins start adding icons next to the Cog, I do think we should apply a fill color to them, to ensure some consistency, and we can't rely on those icons including the dashicon class.

The code example brought up in #5352 (comment) is also a good example of how we use the markup present in the block toolbars to provide button hit areas that are the entire height of the block toolbar and has no empty space between the buttons, yet still visually provides focus styles that are inside those buttons. Although that CSS could be refactored to spell out & > .dashicon as an enhancement, there doesn't seem to be anything wrong with styling SVG markup differently based on context.

As such, while I do agree the initial selector (body.gutenberg-editor-page svg { ... }) is a bit aggressive, I'm not sure the solution is to scope it to the .dashicons class.

Perhaps we can do this:

I submit an upstream dashicons PR to add a helper style.css to the component we import. Dashicons could theoretically be published on npm in the future, and we could then just import it from there.
We duplicate those styles for the Editor Bar, and any other place where plugins provide SVG icons, such as the More menu, so that SVG icons without the dashicons class also get these rules.

Would that address issues on both sides?

afercia · 2018-03-07T11:59:17Z

Have you considered to use a different, additional CSS class to target the SVG icons that should get a "Gutenberg look and feel"? I was thinking at that, as .dashicon is too generic.

Also: plugins may want to use SVGs not only as illustrations but also for their own icons: some icons may need the Gutenberg style, other ones may not and may have a completely different purpose.

jasmussen · 2018-03-07T12:04:30Z

Have you considered to use a different, additional CSS class to target the SVG icons that should get a "Gutenberg look and feel"? I was thinking at that, as .dashicon is too generic.

The fill class recolors the entire SVG to a single flat uniform color. This should work for plugins that do not add a class to their icons at all, for the icons they add to the toolbar or the More menu.

Either that, or we decide that it's fine that plugins use any color they like for these icons, which would add distraction to the editing environment. To me this is the same design pattern as adding top level menu items to WordPress — there are rules here, you use single color icons to ensure a consistent look and feel.

afercia · 2018-03-07T12:19:39Z

I have no objections for icons that are meant to be part of the Gutenberg UI (toolbar, more menu, etc.) I'm all for ensuring a consistent look and feel.

I could argue that in any case any plugin can always override the default look and feel using selectors with high specificity: #myplugin #my-icon { fill: red !important } and they can do this (and they do) also in the current admin menu.

However, plugins have their own content and their own icons. Assuming all plugins icons have to be the same color of the surrounding text (currentColor) is a too broad assumption.

The ones in the screenshot below are all SVGs, and they shouldn't be gray 😉 They will be SVG also when the meta box will live in the sidebar. This applies to any plugin, since SVGs will be used more and more in the future.

jasmussen · 2018-03-07T12:24:13Z

I could argue that in any case any plugin can always override the default look and feel using selectors with high specificity: #myplugin #my-icon { fill: red !important } and they can do this (and they do) also in the current admin menu.

Certainly, but no reason to make it easy.

The ones in the screenshot below are all SVGs, and they shouldn't be gray 😉 They will be SVG also when the meta box will live in the sidebar. This applies to any plugin, since SVGs will be used more and more in the future.

They certainly shouldn't, and this issue is definitely in need of fixing. What I suggested was to submit an upstream fix for icons with the dashicons class, remove the generic SVG fill rule, and then reapply it in a very tight scope, such as in the editor bar or the More menu.

afercia · 2018-03-07T14:13:34Z

Thanks, limiting the scope of the original selector would be awesome.

afercia · 2018-03-23T11:10:24Z

@jasmussen I think this can be closed, correct?

jasmussen · 2018-03-23T11:15:34Z

Yes, I think so.

afercia · 2018-03-23T11:41:57Z

Superseded by #5496

More specific CSS selector for SVG icons.

ed3650c

afercia requested a review from jasmussen March 2, 2018 15:33

Remove tabindex from block mover icons.

615a111

aduth reviewed Mar 6, 2018

View reviewed changes

This was referenced Mar 8, 2018

Add helper SCSS file to React component WordPress/dashicons#280

Merged

More specific CSS selector for SVG icons, round 2 #5496

Merged

afercia closed this Mar 23, 2018

aduth deleted the update/svg-css-selector branch January 25, 2019 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More specific CSS selector for SVG icons. #5352

More specific CSS selector for SVG icons. #5352

afercia commented Mar 2, 2018

afercia commented Mar 5, 2018

aduth Mar 6, 2018

afercia Mar 6, 2018

aduth Mar 6, 2018

afercia Mar 6, 2018

aduth Mar 6, 2018

afercia Mar 6, 2018

aduth Mar 6, 2018

afercia Mar 6, 2018

aduth Mar 6, 2018

afercia Mar 7, 2018

jasmussen commented Mar 6, 2018

afercia commented Mar 6, 2018

jasmussen commented Mar 7, 2018

afercia commented Mar 7, 2018

jasmussen commented Mar 7, 2018

afercia commented Mar 7, 2018

jasmussen commented Mar 7, 2018

afercia commented Mar 7, 2018

afercia commented Mar 23, 2018

jasmussen commented Mar 23, 2018

afercia commented Mar 23, 2018

More specific CSS selector for SVG icons. #5352

More specific CSS selector for SVG icons. #5352

Conversation

afercia commented Mar 2, 2018

Description

How Has This Been Tested?

afercia commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Mar 6, 2018

afercia commented Mar 6, 2018

jasmussen commented Mar 7, 2018

afercia commented Mar 7, 2018

jasmussen commented Mar 7, 2018

afercia commented Mar 7, 2018

jasmussen commented Mar 7, 2018

afercia commented Mar 7, 2018

afercia commented Mar 23, 2018

jasmussen commented Mar 23, 2018

afercia commented Mar 23, 2018