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

Support for style element #1150

Closed
wants to merge 2 commits into from
Closed

Conversation

iqqmuT
Copy link
Contributor

@iqqmuT iqqmuT commented Oct 17, 2019

Support for style element

Not implemented (yet):

  • Support for CSS selectors containing spaces
  • CSS style sheet placed inside a CDATA block

Test Plan

What's required for testing (prerequisites)?

SVG images containing <style> element.

Compatibility

OS Implemented
iOS
Android

Checklist

  • I have tested this on a simulator

@LuizFBrisighello
Copy link

I would like to test this PR, but I'm a beginner and that's my first time exploring the open source world.
What should I do to be able to test it?

@msand
Copy link
Collaborator

msand commented Oct 17, 2019

@LuizFBrisighello You can e.g. do this in the root of your react-native project:

yarn add react-native-svg@iqqmuT/react-native-svg#bf3d7d4
react-native run-ios
react-native run-android

But I'm not sure if I want to bring in any run-time dependencies at this point. What kind of css coverage is needed, is probably quite user/project dependant. Just implementing e.g. the cascade correctly is a massive undertaking. I'd rather expose a hook to allow modifying the AST before it's converted to jsx, in the specific place @iqqmuT is hooking into right now. Alternatively, if you need full css support, use a WebView to display the svg instead.

@msand
Copy link
Collaborator

msand commented Oct 17, 2019

@LuizFBrisighello There's also this: #1074 (comment)

@LuizFBrisighello
Copy link

@msand Thanks a lot for the suggestions, I'll take a deeper look at your comment at #1074 and get back with updates.

@iqqmuT
Copy link
Contributor Author

iqqmuT commented Oct 17, 2019

A hook to allow modifying the AST sounds like a good idea! How about removing the dependency to css module and implementing integrated, simple CSS parser? Most often SVGs that contain <style> element are using only single class selectors (made with Illustrator, Inkscape, etc). It would be nice to provide CSS support for the most SVG files out of the box without compromising the performance. To have better CSS coverage, the AST modifier hook could be used, or a WebView if performance is not an issue.

@msand
Copy link
Collaborator

msand commented Oct 17, 2019

@iqqmuT Sounds like an acceptable tradeoff, but I'd still like to keep the plain version without any css processing, and introduce e.g. a SvgCssXml element for minimal css processing as a showcase for how to use the ast middleware hook. What do you think of this?

@iqqmuT
Copy link
Contributor Author

iqqmuT commented Oct 17, 2019

Introducing a new element supporting CSS sounds like a brilliant idea!

About implementing a css parser, the one in css module is:

  • thoroughly tested: 3 millions weekly downloads
  • stable: hasn't changed for almost 4 years
  • MIT licensed

I wonder if the parser source could be copied into this project to get rid of the dependency.
https://github.com/reworkcss/css/blob/master/lib/parse/index.js

@msand
Copy link
Collaborator

msand commented Oct 17, 2019

@iqqmuT There's also e.g. CSSTree which seems quite fast: https://github.com/csstree/csstree
Used by e.g. CSSO and SVGO
https://github.com/postcss/benchmark#parsers

@msand
Copy link
Collaborator

msand commented Oct 19, 2019

@msand
Copy link
Collaborator

msand commented Oct 20, 2019

Tried implementing something which would handle the cascade / specificity / priorities correctly: https://github.com/react-native-community/react-native-svg/tree/css

Can still be optimized a lot, this is just a first proof of concept.

@msand
Copy link
Collaborator

msand commented Oct 20, 2019

5f3852b

@msand
Copy link
Collaborator

msand commented Oct 20, 2019

Example:

App.js

import React from 'react';
import {TouchableOpacity} from 'react-native';
import {parse} from 'react-native-svg';
import {inlineStyles, SvgCss} from 'react-native-svg/src/css';
import {svgWithCssExample} from './test';

const App = () => {
  return (
    <TouchableOpacity
      style={{flex: 1, backgroundColor: 'blue'}}
      onPress={() => {
        const parsed = parse(svgWithCssExample, x => {
          let styled = inlineStyles(x);
          console.log({...styled});
          return styled;
        });
        console.log(parsed);
      }}>
      <SvgCss xml={svgWithCssExample} />
    </TouchableOpacity>
  );
};

export default App;

@iqqmuT
Copy link
Contributor Author

iqqmuT commented Oct 20, 2019

Awesome, that seems to work, thank you! Are you considering using run-time dependencies?

Just a small tip for supporting CSS properties with a dash, they should probably be camelCased:

.class1 {
  stroke-width: 10;
}

This should be set as style strokeWidth. Something like this:

diff --git a/src/css.js b/src/css.js
index e4d5a75..f331f2f 100644
--- a/src/css.js
+++ b/src/css.js
@@ -1,5 +1,5 @@
 import React, { useEffect, useMemo, useState } from 'react';
-import { parse, SvgAst } from './xml';
+import { camelCase, parse, SvgAst } from './xml';
 import baseCssAdapter from 'css-select-base-adapter';
 import csstree, { List } from 'css-tree';
 import cssSelect from 'css-select';
@@ -764,7 +764,7 @@ export function inlineStyles(document) {
             return;
           }
           selectedEl.style.setProperty(
-            styleDeclaration.name,
+            camelCase(styleDeclaration.name),
             styleDeclaration.value,
             styleDeclaration.priority,
           );

@msand
Copy link
Collaborator

msand commented Oct 20, 2019

@iqqmuT Ah great, thanks for spotting it! I made a separate pr here: #1155
I'm thinking in this case it might be worth the tradeoff to have some dependencies at first. Just have to make sure that the web bundle doesn't include them. Ideally we would vendor the needed packages, strip out any logic we're not using, remove any options, and optimize the remaining implementation. I'm thinking of making a minimal version of CSSStyleDeclaration first, as the current one is much heavier than what's needed in this case.

@iqqmuT
Copy link
Contributor Author

iqqmuT commented Oct 20, 2019

Sounds really good. I close this PR.

@iqqmuT iqqmuT closed this Oct 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants