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.

Sign up for GitHub

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

More specific CSS selector for SVG icons. #5352

Closed
wants to merge 2 commits into from
Closed

More specific CSS selector for SVG icons. #5352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed3650c
More specific CSS selector for SVG icons.
afercia Mar 2, 2018
615a111
Remove tabindex from block mover icons.
afercia Mar 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions edit-post/assets/stylesheets/main.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,6 @@ body.gutenberg-editor-page {
top: -1px;
}

svg {
fill: currentColor;
outline: none;
}

ul#adminmenu a.wp-has-current-submenu:after,
ul#adminmenu>li.current>a.current:after {
border-right-color: $white;
Expand Down Expand Up @@ -104,6 +99,11 @@ body.gutenberg-editor-page {
font-size: $default-font-size;
color: $dark-gray-500;
}

svg.dashicon {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

I've just added .dashicon 🙂

This is the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

fill: currentColor;
outline: none;
}
}

.gutenberg__editor {
Expand Down
4 changes: 2 additions & 2 deletions editor/components/block-mover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<path d="M12.293 12.207L9 8.914l-3.293 3.293-1.414-1.414L9 6.086l4.707 4.707z" />
</svg>
);

const downArrow = (
<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">
<path d="M12.293 6.086L9 9.379 5.707 6.086 4.293 7.5 9 12.207 13.707 7.5z" />
</svg>
);
Expand Down