Add an eslint rule to use cross-environment SVG primitives by youknowriad · Pull Request #10652 · WordPress/gutenberg

youknowriad · 2018-10-16T15:34:28Z

Still WIP, the idea is to try to force the usage of the SVG primitives instead of the raw components to ensure mobile compatibility.

I don't like that this is forcing us to add a dependency: blocks -> components. Makes me think these primitives should be better in the element package.
The eslint rule is too aggressive at the moment, it also catches props named "path"

hypest · 2018-10-17T12:04:54Z

.eslintrc.js

 				message: 'Deprecated functions must be removed before releasing this version.',
 			},
+			{
+				selector: 'JSXIdentifier[name=/^(path|svg)$/]',


Should we also add <g> and <polygon> that we already have componentized as well?

Yes, sure.

The eslint rule is too aggressive at the moment, it also catches props named "path"

As Riad mentioned, this rule needs to be updated anyways.

gziolo · 2018-10-17T12:45:55Z

Many thanks for opening this PR @youknowriad ❤️

gziolo · 2018-10-17T15:47:43Z

I don't like that this is forcing us to add a dependency: blocks -> components. Makes me think these primitives should be better in the element package.

<SVG xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><Path d="M19 7h-1V5h-4v2h-4V5H6v2H5c-1.1 0-2 .9-2 2v10h18V9c0-1.1-.9-2-2-2zm0 10H5V9h14v8z" /></SVG>

I personally think that @wordpress/blocks shouldn't be so opinionated about the default icon. If we could register a default icon instead it would solve the issue. It could be similar to:

setDefaultBlockName( paragraph.name );
if ( window.wp && window.wp.oldEditor ) {
	setFreeformContentHandlerName( classic.name );
}
setUnregisteredTypeHandlerName( missing.name );
...
setDefaultIcon( 
    <SVG xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><Path d="M19 7h-1V5h-4v2h-4V5H6v2H5c-1.1 0-2 .9-2 2v10h18V9c0-1.1-.9-2-2-2zm0 10H5V9h14v8z" /></SVG>
);

gziolo · 2018-10-23T07:55:24Z

@Tug if you have some time, you can help to land this one.

gziolo · 2018-10-23T10:09:42Z

This should be ready to ship. I figured out that there is an existing rule in react linting library which we could take advantage of: https://github.com/yannickcr/eslint-plugin-react/blob/HEAD/docs/rules/forbid-elements.md.

aduth

This white-screens for me (and apparently in Travis) with the following error:

Uncaught TypeError: Cannot read property 'SVG' of undefined

Maybe missing a dependency? (From blocks to components)

gziolo · 2018-10-24T06:32:36Z

Maybe missing a dependency? (From blocks to components)

Yes, it is an actual issue which @youknowriad raised earlier. I will refactor code to avoid using SVG inside @wordpress/blocks package. It should be UI agnostic.

gziolo · 2018-10-24T08:31:28Z

packages/editor/src/components/block-icon/index.js


 function renderIcon( icon ) {
 	if ( 'string' === typeof icon ) {
+		if ( icon === 'block-default' ) {


@jasmussen - is it okay to handle SVG replacement in here?

We want to avoid having a dependency on @wordpress/components inside @wordpress/blocks. My reasoning is as follows: blocks API should be UI agnostic as much as possible, that's why it provides only an icon name. BlockIcon is always used to render an actual icon, so we can be more opinionated here.

Can you elaborate a little bit? Not sure I understand the context.

See also my previous comment: #10652 (comment).

I changed my mind when entertaining with the exisitng implementation. I reverted all change in normalizeIconObject to use default-block dashicon. However, it is replaced with the new nice SVG icon inside BlockEdit. This should behave exactly the same in the UI.

Ah, gotcha. I don't have strong opinions here, and I would defer to you two for the technical implementation. What matters is:

We encourage the use of SVG (i.e. make it easy)

We can provide an icon set even for new blocks, like Dashicons, in the future.

3rd party provided SVGs are processed so role and focusable attributes are added.

If those values are upheld, 👍 👍 from me.

We can provide an icon set even for new blocks, like Dashicons, in the future.

I think it would be convenient to be able to use strings if someone is happy with the default icon set.

3rd party provided SVGs are processed so role and focusable attributes are added.

This is what SVG component does :)

gziolo · 2018-10-24T08:55:42Z

I don't like that this is forcing us to add a dependency: blocks -> components. Makes me think these primitives should be better in the element package.

It's no longer the case, see #10652 (comment).

This white-screens for me (and apparently in Travis) with the following error:
Uncaught TypeError: Cannot read property 'SVG' of undefined
Maybe missing a dependency? (From blocks to components)

e2e tests pass so I think block icons should be fine as @jorgefilipecosta build an extensive set of assertions for them. It was missing dependency, but I solved it by making @wordpress/blocks components agnostic.

gziolo · 2018-10-24T09:18:38Z

I added fbd3a30 to ensure that fix for the regression (#10987) works with the default SVG icon, too.

mzorz

great work @gziolo - there's a conflict to be resolved in packages/editor/src/components/block-icon/index.js but otherwise LGTM after that one gets fixed

Error addressed

Uses an existing react eslint rule to perform validation. Adds new components to make the existing icons mobile app friendly.

@jasmussen

… normalization in BlockIcon Props to @jasmussen for helping to catch it.

@jasmussen

…#10652) * Add an eslint rule to use cross-environment SVG primitives * Extend the list of forbidden elements Uses an existing react eslint rule to perform validation. Adds new components to make the existing icons mobile app friendly. * Change the way the default icon for block is set * Ensure that default icon goes through the process of width and height normalization in BlockIcon Props to @jasmussen for helping to catch it.

youknowriad added the [Type] Code Quality Issues or PRs that relate to code quality label Oct 16, 2018

youknowriad requested review from aduth and gziolo October 16, 2018 15:34

hypest reviewed Oct 17, 2018

View reviewed changes

gziolo added this to the 4.2 - API freeze milestone Oct 17, 2018

gziolo mentioned this pull request Oct 23, 2018

Embed block tidy #10793

Merged

4 tasks

gziolo added Mobile Web Viewport sizes for mobile and tablet devices Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) and removed Mobile Web Viewport sizes for mobile and tablet devices labels Oct 23, 2018

gziolo force-pushed the try/removing-svg-elements branch from 37533b2 to 7f8df8c Compare October 23, 2018 10:07

gziolo requested review from a team and mzorz October 23, 2018 10:10

gziolo assigned gziolo and youknowriad Oct 23, 2018

aduth previously requested changes Oct 23, 2018

View reviewed changes

gziolo force-pushed the try/removing-svg-elements branch from 7f8df8c to 6efe432 Compare October 24, 2018 08:29

gziolo reviewed Oct 24, 2018

View reviewed changes

gziolo requested a review from jorgefilipecosta October 24, 2018 08:40

gziolo requested a review from aduth October 24, 2018 08:55

mzorz approved these changes Oct 24, 2018

View reviewed changes

youknowriad and others added 2 commits October 25, 2018 07:55

Add an eslint rule to use cross-environment SVG primitives

55a69b6

Extend the list of forbidden elements

34aeb49

Uses an existing react eslint rule to perform validation. Adds new components to make the existing icons mobile app friendly.

gziolo added 2 commits October 25, 2018 07:58

Change the way the default icon for block is set

26eb746

Ensure that default icon goes through the process of width and height…

0b37479

… normalization in BlockIcon Props to @jasmussen for helping to catch it.

gziolo force-pushed the try/removing-svg-elements branch from fbd3a30 to 0b37479 Compare October 25, 2018 06:01

gziolo merged commit 578d0ef into master Oct 25, 2018

gziolo deleted the try/removing-svg-elements branch October 25, 2018 09:04

Conversation

youknowriad commented Oct 16, 2018

Uh oh!

hypest Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo commented Oct 17, 2018

Uh oh!

gziolo commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gziolo commented Oct 23, 2018

Uh oh!

gziolo commented Oct 23, 2018

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

gziolo commented Oct 24, 2018

Uh oh!

gziolo Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasmussen Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

jasmussen Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo commented Oct 24, 2018

Uh oh!

gziolo commented Oct 24, 2018

Uh oh!

mzorz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gziolo commented Oct 17, 2018 •

edited

Loading

gziolo Oct 24, 2018 •

edited

Loading