React Native - SVG icons for Inserter #9294

mzorz · 2018-08-24T01:34:14Z

Description

This PR adds mobile SVG compatibility in React Native for SVG icons of already ported blocks:

Code
Paragraph
More
Heading

How has this been tested?

Using this PR in the Gutenberg mobile repo

Screenshots

Types of changes

This PR

adds support for the SVG <g> tag in the for of G as needed by our wrapper library
changes the icon: prop declaration for the aforementioned blocks to use the wrapper <SVG tag as follows:

What was til now declared as:

	icon: <svg viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"><path fill="none" d="M0 0h24v24H0V0z" /><g><Path d="M2 9v2h19V9H2zm0 6h5v-2H2v2zm7 0h5v-2H9v2zm7 0h5v-2h-5v2z" /></g></svg>,

is now declared as:

	icon: <SVG viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"><Path fill="none" d="M0 0h24v24H0V0z" /><G><Path d="M2 9v2h19V9H2zm0 6h5v-2H2v2zm7 0h5v-2H9v2zm7 0h5v-2h-5v2z" /></G></SVG>,

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

…ading, more and paragraph blocks

…idth do not have to be indicated explicitely

gziolo · 2018-08-24T04:54:29Z

There are some linting issues which prevents this PR from being merged. You can see them on Travis: https://travis-ci.org/WordPress/gutenberg/jobs/419924102

[0] [0] /home/travis/build/WordPress/gutenberg/packages/block-library/src/code/index.js
[0] [0]   21:1  error  Trailing spaces not allowed  no-trailing-spaces
[0] [0] 
[0] [0] /home/travis/build/WordPress/gutenberg/packages/block-library/src/heading/index.js
[0] [0]   24:3  error  Trailing spaces not allowed  no-trailing-spaces
[0] [0] 
[0] [0] /home/travis/build/WordPress/gutenberg/packages/components/src/primitives/svg/index.native.js
[0] [0]   10:5   error  There must be a space inside this paren                   space-in-parens
[0] [0]   10:18  error  Expected '!==' and instead saw '!='                       eqeqeq
[0] [0]   10:47  error  Expected '!==' and instead saw '!='                       eqeqeq
[0] [0]   10:59  error  There must be a space inside this paren                   space-in-parens
[0] [0]   16:9   error  Unnecessary 'else' after 'return'                         no-else-return
[0] [0]   19:7   error  'viewBoxCoords' is never reassigned. Use 'const' instead  prefer-const
[0] [0]   19:42  error  There must be a space inside this paren                   space-in-parens
[0] [0]   19:46  error  There must be a space inside this paren                   space-in-parens
[0] [0]   21:30  error  A space is required after '['                             computed-property-spacing
[0] [0]   21:32  error  A space is required before ']'                            computed-property-spacing
[0] [0]   21:58  error  A space is required after '['                             computed-property-spacing
[0] [0]   21:60  error  A space is required before ']'                            computed-property-spacing

gziolo · 2018-08-24T05:06:07Z

@mtias @jasmussen - out of curiosity, how are those new SVG icons generated? I'm asking because we need to take into account the needs of React Native environment moving forward where we will have to use imported primitive components which allow to branch code between web and mobile.

gziolo · 2018-08-24T05:11:14Z

packages/components/src/primitives/svg/index.native.js

+				{ props.children }
+			</Svg>
+		);
+	} else {


Svg implementation handles viewBox out of the box:
https://github.com/react-native-community/react-native-svg/blob/master/elements/Svg.js#L35

That's right viewBox is handled, but the component will fail to render if height and width are not set - what this (most probably hack-ish) piece of code does is fabricate the height and width out of the information passed in viewBox which seemed to exist in all cases for all blocks in code (I relied on that to process unless width and height are explicitely set through props). For this I separated this specific change in commit c75a0db wdyt?

gziolo · 2018-08-24T05:14:34Z

packages/components/src/primitives/svg/index.native.js

 	Svg,
 } from 'react-native-svg';

 export const Path = PATH;
+export const G = GROUP;


You don't need to alias those components, you can do the following instead:

import { G, Path, Svg } from 'react-native-svg'; export { G, Path, };

addressed in bf65beb

gziolo · 2018-08-24T05:17:21Z

packages/components/src/primitives/svg/index.native.js

+		// take viewport system to match the viewBox definition
+		// i.e. viewBox="0 0 24 24"
+		let viewBoxCoords = props.viewBox.split(' ');
+		return (


I think @SergioEstevao had an issue with one of the passed props. I was wondering if usin omit from 'lodash' would work here:

export const SVG = ( props ) => { return <Svg { ...omit( props, [ 'className' ] } />; };

That was the problem, RN complained about className being set. if omit takes it away it could work :).

I think this is going to be a recurring issue, it would be nice to find a way to skip those className properties or convert them to style. Ideally, it would work out of the box.

tested it a bit and it seems to be working 👍
addressed in 6d5b37d

gziolo · 2018-08-24T05:18:59Z

packages/block-library/src/code/index.js

@@ -8,6 +8,7 @@ import { createBlock } from '@wordpress/blocks';
 * Internal dependencies
 */
 import edit from './edit';
+import { Path, SVG } from '@wordpress/components';


It should be put under WordPress dependencies section. See related coding guidelines: https://github.com/WordPress/gutenberg/blob/master/docs/reference/coding-guidelines.md#imports.

Thanks! addressed in ed725d6

gziolo · 2018-08-24T05:19:04Z

packages/block-library/src/heading/index.js

@@ -20,7 +20,8 @@ import { RichText } from '@wordpress/editor';
 * Internal dependencies
 */
 import edit from './edit';
-
+import { Path, SVG } from '@wordpress/components';


The same thing with WordPress dependencies section.

addressed in ed725d6

gziolo · 2018-08-24T05:19:09Z

packages/block-library/src/more/index.js

@@ -14,6 +14,7 @@ import { createBlock } from '@wordpress/blocks';
 * Internal dependencies
 */
 import edit from './edit';
+import { Path, G, SVG } from '@wordpress/components';


The same thing with WordPress dependencies section :)

same as above

gziolo · 2018-08-24T05:19:17Z

packages/block-library/src/paragraph/index.js

@@ -25,6 +25,7 @@ import {
 * Internal dependencies
 */
 import edit from './edit';
+import { Path, SVG } from '@wordpress/components';


The same thing with WordPress dependencies section 😅

:) same as above, addressed in ed725d6

jasmussen · 2018-08-24T09:08:49Z

out of curiosity, how are those new SVG icons generated? I'm asking because we need to take into account the needs of React Native environment moving forward where we will have to use imported primitive components which allow to branch code between web and mobile.

That's the thing. We can't know. Because plugins add these, so they could be using horribly mangled SVGs.

As is also discussed in #9269 (comment), this also has accessibility implications.

Do we have any way to build a tool to parse the SVG source files and serve a sanitized version on the fly?

gziolo · 2018-08-24T09:52:24Z

That's the thing. We can't know. Because plugins add these, so they could be using horribly mangled SVGs.

I wouldn't worry about it too much. We didn't even start discussing what needs to happen to enable plugins on mobile. I can imagine this is going to be opt-in because you will have to operate on a much more constrained environment. You won't be able to use DOM element as they are. SVG is just and example.

Do we have any way to build a tool to parse the SVG source files and serve a sanitized version on the fly?

I don't think there is one, however it isn't that complex. In the majority of cases, the only thing you need to change is to capitalize tag name. SVG is the only exception because it needs to follow our coding guidelines. Please also note that you can't fully copy <svg /> as is today, you always need to convert class to className to make it work with React.

…own to Svg component

- Fix an issue where eventCount was not correctly propogated to AztecText resulting in a re-rendering of the component from scratch

mzorz · 2018-08-24T14:56:17Z

Added stripping style for <svg> native implementation as discussed elsewhere, commit has comment explaining 516f661.

Ready for another round @gziolo 🙇

…svgicons-take1

hypest · 2018-09-04T16:20:53Z

packages/components/src/primitives/svg/index.native.js

 export const SVG = ( props ) => {
+	// for some reason if a `className` prop is passed it gets converted to `style` here.


The reason the className gets transformed into style is because on the RN side, the babel-plugin-react-native-classname-to-style is active. That was part of the work done in order to support SCSS on the RN app. See this PR.

What exactly is the problem when style appears? Is the Svg component not using it or does it choke on it?

citing the comment: Given it carries a string (as it was originally className) but an object is expected for "style"... it chokes :)

Also thanks for pointing that out (about the PR where this change is implemented). We discussed that elsewhere and forgot to add this here. It might be good to change the comment to add this information in particular, adjusting the "for some reason..." part.

Comment modified in 5227859

… to style for RN

hypest · 2018-09-05T07:56:39Z

packages/components/src/primitives/svg/index.native.js

+	// We're using the react-native-classname-to-style plugin, so when a `className` prop is passed it gets converted to `style` here.
+	// Given it carries a string (as it was originally className) but an object is expected for `style`,
+	// we need to check whether `style` exists and is a string, and strip in that case.
+	const safeProps = ( typeof props.style === 'string' || props.style instanceof String ) ? { ...omit( props, [ 'style', 'className' ] ) } : { ...omit( props, [ 'className' ] ) };


Feels super weird to have to manually strip the style definition so, if you don't mind @mzorz , let's dig a little deeper in this one.

We already had a chat over Slack with what probably needs to be done here (properly load the styles for RN) so, let's wait a bit to finish that effort, is that OK?

As discussed with @hypest , made the style convert from string to object properly (if it applies) in 2d47451 and b678c6d.

To test, I added the following:

.dashicons-editor-bold { transform: rotate(20deg); }

to packages/components/src/dashicon/style.scss

and the styles get passed to the svg component, this is how the bold icon gets rendered:

See the B icon slightly leaning to the right? 😄

Out of curiosity, why does it happen that style becomes a string?
On the web you have to pass an object as style, I think this is also the case in React Native. So this is very surprising. I would love to understand why this is something we need to handle in the first place.

As far as I understand, className is not supported in React Native, so we need to define that as a prop on each object we'd want to use that in.
Because of this and being that className is being used in different places in Gutenberg, we're using a Babel plugin that transforms className to style in RN so to make it transparent to Gutenberg code.

AFAIU the plugin just keeps the passed object as it was without making any other conversion, so if the original object was a string, it will be a string after going through it as well.

So knowing this, it looks like the problem arises then because Dashicon gutenberg/packages/components/src/dashicon/index.js declares className={ iconClass } where iconClass is a string, a defined here: https://github.com/WordPress/gutenberg/blob/master/packages/components/src/dashicon/index.js#L899

While className can be a string (as it originally is in Dashicon code), style should be an object instead, but we're not transforming it (just keeping the same type).

Does that make sense? Should we do something differently? cc @hypest as well to see if there's something else to add / do differently in this regard.

…ing on svgnative implementation

gziolo · 2018-09-06T14:10:07Z

This needs to be synced with the web implementation. We discussed it a few times with @jasmussen and there is no clear way to move forward. It would be great if we could figure out a solution where we would be able to use SVG as is generated and convert it to React element on the fly, for web it would be possible with this Babel plugin:
https://github.com/airbnb/babel-plugin-inline-react-svg/blob/master/README.md. It looks like there is a bit different solution for React Native: https://github.com/vault-development/react-native-svg-uri, but maybe we could find something that would work on both platforms.

hypest · 2018-09-06T15:06:02Z

@gziolo I wonder, can we perhaps abstract away the value of the icon block field into a function and then nativize that function? On the web case that function would return a string (dashicon name) or the lowercase <svg React tree, while on the mobile case it will use the <Svg variant. Can't that be a path forward for the time being?

koke · 2018-09-07T06:24:58Z

I'm curious, what's the reasoning behind inlining SVGs? Could we keep them as a file and use require to load them? Then each platform could use a different loader for them, right?

gziolo · 2018-09-07T07:36:13Z

I'm curious, what's the reasoning behind inlining SVGs? Could we keep them as a file and use require to load them? Then each platform could use a different loader for them, right?

It's more or less the same, not going into detail, it would work on a higher level as you described.

jasmussen · 2018-09-07T09:06:32Z

I'm curious, what's the reasoning behind inlining SVGs? Could we keep them as a file and use require to load them? Then each platform could use a different loader for them, right?

In the rendered code, we need the SVGs to be inline so we can colorize them using fill CSS properties. But I'm sort of assuming that subtext of your note implies this would still be the case if we loaded SVG content from files.

jasmussen · 2018-09-07T09:07:38Z

One of the challenges we are also facing (see #9565) is that we need a few attributes added to the SVG, which we can't assume are always there.

gziolo · 2018-09-07T10:36:49Z

I'm playing with an alternative approach in #9685. The idea is as follows:

To make it work with React Native platform, I want to overload createElement to use react-native-svg elements instead of HTML types:

svg -> Svg (from react-native-svg)

path -> Path

g -> G

and so on

This would be applied only to the RN version of react implementation.

…vgicons-take1

hypest · 2018-10-09T09:30:39Z

From #9685 (comment):

Okey, let's merge this one and rebase #9294 against master.

@mzorz , let's try updating from master here and see where this goes, wdyt?

gziolo · 2018-10-10T08:22:24Z

Yes, let's try to use primitives I exposed in #9685 while I experiment with adding support for HTML tags. I didn't make it work so far and the more I think about it, the more it seems like having abstraction layer is a more elegant solution. In particular, it might be more reliable when using with all 3rd party libraries which use React directly. I think this is the biggest challenge to ensure that createElement is properly proxied with our own implementation.

mzorz · 2018-10-11T21:58:32Z

@mzorz , let's try updating from master here and see where this goes, wdyt?

Just finished updating this branch to master - also updated the related Gutenberg mobile PR at wordpress-mobile/gutenberg-mobile#129
@hypest @gziolo

…act Native compatibility

hypest

LGTM! Feel free to merge as soon as Travis gives the green light too!

mzorz added 2 commits August 23, 2018 21:58

exports SVG <G> tag and adds mobile compatible SVG icons for code, he…

423d189

…ading, more and paragraph blocks

adds simple mechanism to obtain viewport from viewBox so height and w…

c75a0db

…idth do not have to be indicated explicitely

mzorz mentioned this pull request Aug 24, 2018

Inserter - block icons wordpress-mobile/gutenberg-mobile#129

Merged

mzorz requested review from SergioEstevao and gziolo August 24, 2018 01:35

gziolo added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Aug 24, 2018

gziolo added this to the 3.7 milestone Aug 24, 2018

gziolo reviewed Aug 24, 2018

View reviewed changes

SergioEstevao and others added 7 commits August 24, 2018 11:07

Update use of content size change event.

cb7c413

fixed lint issues

eb9d683

removed unneeded aliases

bf65beb

using omit from loadsh to exclude className from props when passing d…

6d5b37d

…own to Svg component

moved imports to their corresponding section

ed725d6

- Force update the RichText component when a toolbar button is pressed

1b5653a

- Fix an issue where eventCount was not correctly propogated to AztecText resulting in a re-rendering of the component from scratch

stripping style if not an object in svg native implementation

516f661

Merge branch 'master' into rnmobile/inserter-svgicons-take1

a314ad6

mzorz mentioned this pull request Aug 24, 2018

Inserter wordpress-mobile/gutenberg-mobile#58

Closed

4 tasks

mzorz added 5 commits August 24, 2018 12:27

Merge commit 'cb7c413' into HEAD

994993c

Merge branch 'fix/icon-svg-gb-hash-reference' into rnmobile/inserter-…

b0103ea

…svgicons-take1

added empty native implementation of viewport

b09a59d

fixed lint issues

93513d0

fixed merge conflict

c1cb4e5

youknowriad modified the milestones: 3.7, 3.8 Aug 30, 2018

Merge commit '1b5653a' into rnmobile/inserter-svgicons-take1-merge

2886e01

hypest reviewed Sep 4, 2018

View reviewed changes

Modified comment to better reflect causes of className being conveted…

5227859

… to style for RN

hypest reviewed Sep 5, 2018

View reviewed changes

youknowriad removed this from the 3.8 milestone Sep 5, 2018

building an object out of the passed style/className concatenated str…

2d47451

…ing on svgnative implementation

gziolo requested review from youknowriad and jasmussen September 6, 2018 14:06

fix array level indirection when looking into styles object

b678c6d

gziolo mentioned this pull request Sep 6, 2018

Rename the AccessibleSVG component just Svg #9656

Closed

gziolo mentioned this pull request Sep 7, 2018

Rename AccessibleSVG to SVG and make it work with React Native #9685

Merged

5 tasks

Merge remote-tracking branch 'origin/master' into rnmobile/inserter-s…

d15300f

…vgicons-take1

fixed merge conflicts

8177f81

enforcing usage of SVG and Path primitives in image icon field for Re…

defd31c

…act Native compatibility

hypest approved these changes Oct 12, 2018

View reviewed changes

mzorz merged commit b255cd6 into master Oct 12, 2018

mzorz deleted the rnmobile/inserter-svgicons-take1 branch October 12, 2018 12:48

mzorz mentioned this pull request Oct 12, 2018

[RN mobile] fix className style in SVG primitive #10556

Merged

4 tasks

marecar3 mentioned this pull request Nov 5, 2018

Importing .scss file doesn't work when it isn't in the same directory as file which imports it #11487

Closed

aduth mentioned this pull request Mar 26, 2020

SVG primitive: Support custom aria-hidden value #21173

Closed

6 tasks

		export const SVG = ( props ) => {
		// for some reason if a `className` prop is passed it gets converted to `style` here.

React Native - SVG icons for Inserter #9294

React Native - SVG icons for Inserter #9294

Conversation

mzorz commented Aug 24, 2018 • edited

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

gziolo commented Aug 24, 2018

gziolo commented Aug 24, 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

gziolo Aug 24, 2018 • edited

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 Aug 24, 2018

gziolo commented Aug 24, 2018

mzorz commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzorz Sep 4, 2018 • edited

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

gziolo commented Sep 6, 2018

hypest commented Sep 6, 2018 • edited

koke commented Sep 7, 2018

gziolo commented Sep 7, 2018

jasmussen commented Sep 7, 2018

jasmussen commented Sep 7, 2018

gziolo commented Sep 7, 2018

hypest commented Oct 9, 2018

gziolo commented Oct 10, 2018

mzorz commented Oct 11, 2018

hypest left a comment

Choose a reason for hiding this comment

mzorz commented Aug 24, 2018 •

edited

gziolo Aug 24, 2018 •

edited

mzorz Sep 4, 2018 •

edited

hypest commented Sep 6, 2018 •

edited