Fix component style attribute parser #45

ianvieira · 2017-10-25T00:43:22Z

When we have a style attribute on the original HTML, the parsing was not correct, specially with background-image with an external URL, this was not loading because of a split by ':' what breaks the URL. I added an library that generates a more complete style tag using css-to-react-native.

coveralls · 2017-10-25T00:45:04Z

Coverage decreased (-0.03%) to 99.206% when pulling f678c3f on ianvieira:master into faad529 on remarkablemark:master.

remarkablemark · 2017-10-26T02:11:18Z

Thanks for opening the pull request and bringing the bug to my attention @ianvieira

My question for you is do you think css-to-react-native is too specific? Will it transform the styles correctly if rendered in the DOM? Or does it make sense to roll out a custom parser that is built on top of css?

Also, since html-react-parser is isomorphic, how should the parsing work in the browser? Let me know what you think.

coveralls · 2017-11-23T19:53:15Z

Coverage decreased (-0.04%) to 99.2% when pulling 6146c8a on ianvieira:master into faad529 on remarkablemark:master.

ianvieira · 2017-11-23T19:56:24Z

@remarkablemark After using the new solution with the css-to-react-native, I've found some issues, so I tried another approach, that is more successful. Now I'm using AST to deconstruct the style string and then creating a new object with the values and property name in camel case. This new approach works faster as before and without the problem of using a isomorphic designed module.

What do you think about the new solution?

remarkablemark · 2017-11-26T06:16:26Z

@ianvieira Nice! I like the AST approach because it'll support the general use cases.

The solution looks great but I found something unfortunate about css-tree. The browser library is 165.7 kB minified which is huge.

The alternative css is 10.81 kB unminified.

I have a library style-to-object which is built on top of css. Could this be used instead? You might want to check out iterator.

I have a few more requests (if you would be so kind):

Use conventional commits (I'm trying to automate the CHANGELOG)
Refactor camelCase in ./lib/utilities.js with your method or camelize
- Note: There may be edge cases regarding vendor prefixes
Write a test to ensure that inline styles with urls do not break

I want to thank you once more for looking into this and submitting the pull request. I really appreciate your time and effort. If you feel like you're swamped or have no time, I can always take over and lend a helping hand.

ianvieira · 2017-11-26T18:53:01Z

Of course, I can address these issues, in fact this will change will help me too, my application bundle is with 900kb and I'm trying to make it smaller. I'll do a new commit with the changes and also check if your library handles my biggest problem, the background with a base64 image, which was not being interpreted rightly.

…to an style object

coveralls · 2017-11-26T19:31:24Z

Coverage decreased (-0.08%) to 99.153% when pulling 68cd565 on ianvieira:master into faad529 on remarkablemark:master.

…mel case and assert the string is a string

coveralls · 2017-11-26T19:44:09Z

Coverage decreased (-0.06%) to 99.18% when pulling 4522666 on ianvieira:master into faad529 on remarkablemark:master.

… remove double quote from tests

coveralls · 2017-11-26T19:49:31Z

Coverage decreased (-0.06%) to 99.174% when pulling 10ff149 on ianvieira:master into faad529 on remarkablemark:master.

ianvieira · 2017-11-26T19:51:32Z

@remarkablemark I believe now it's how you expected. I tried to address all your requests and also added an base64 image for test, this way we're able to test this edge case and also the url.

remarkablemark · 2017-11-26T20:18:30Z

@ianvieira Thanks for addressing the requests so quickly. I made a few comments and then let's get this thing merged in.

Oh and before I forget, were you able to verify if the style prefix for -ms works as expected?

remarkablemark · 2017-11-26T20:09:21Z

lib/attributes-to-props.js

@@ -5,6 +5,7 @@
 */
 var utilities = require('./utilities');
 var propertyConfig = require('./property-config');
+var parser = require('style-to-object');


Do you mind renaming parser to styleToObject just for keeping it explicit here?

remarkablemark · 2017-11-26T20:10:51Z

lib/attributes-to-props.js

-    for (var i = 0, declarationsLen = declarations.length; i < declarationsLen; i++) {
-        properties = declarations[i].trim().split(':');
+    parser(style, function(propName, propValue) {
+        styleObj[utilities.camelCase(propName)] = propValue;


One thing about the style parser is that css comments is included in the iterator. Could we include an undefined check here?

if (propName && propValue) { styleObj[utilities.camelCase(propName)] = propValue; }

remarkablemark · 2017-11-26T20:12:15Z

lib/utilities.js

@@ -6,24 +6,19 @@
 * @param  {String} string - The string.
 * @return {String}
 */
+
+var _hyphenPattern = /-(.)/g;


Would you mind moving this line above the JSDoc? This is because we can find easier to read the comment when it's next to the function.

remarkablemark · 2017-11-26T20:13:33Z

package.json

@@ -30,7 +30,8 @@
  ],
  "dependencies": {
    "html-dom-parser": "0.1.2",
-    "react-dom-core": "0.0.2"
+    "react-dom-core": "0.0.2",
+    "style-to-object": "^0.2.0"


Could you remove the ^? I'm following save-exact since I don't have a package-lock.json or yarn.lock checked in.

ianvieira · 2017-11-26T20:20:59Z

I’ll do the requested changes between today and tomorrow and yes I checked the browser prefix and it’s working as expected, I also added this case to the tests.

…

On Sun, 26 Nov 2017 at 18:19 Mark ***@***.***> wrote: @ianvieira <https://github.com/ianvieira> Thanks for addressing the requests so quickly. I made a few comments and then let's get this thing merged in. Oh and before I forget, were you able to verify if the style prefix for -ms works as expected? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-c8Ff8E6CdPtANvfONusH1bzB4F6S-ks5s6ceWgaJpZM4QFSJc> .

ianvieira · 2017-11-27T16:00:43Z

@remarkablemark I finished all the requested changes

coveralls · 2017-11-27T16:02:12Z

Coverage decreased (-0.06%) to 99.18% when pulling 3ca6681 on ianvieira:master into faad529 on remarkablemark:master.

remarkablemark · 2017-11-27T17:07:09Z

@ianvieira Awesome work! Also if you're looking to visualize your webpack bundle stats, check out Webpack Visualizer.

ianvieira · 2017-11-27T17:12:07Z

@remarkablemark Thank you! Great tip! I have another question, when this new version will be available at NPM? Currently I'm using my repo as source and I want to change to the package again

remarkablemark · 2017-11-27T18:44:28Z

@ianvieira I'll try to release either tonight or tomorrow morning.

remarkablemark · 2017-11-28T05:25:37Z

@ianvieira I have released 0.4.1:

# npm
npm install html-react-parser@latest --save

# yarn
yarn upgrade html-react-parser --latest

Fix component style attribute parser

Fix component style attribute parser

f678c3f

remarkablemark added the bug Something isn't working label Oct 26, 2017

Fix component style attribute parser using AST

6146c8a

fix(attributes-to-props.js): Use AST to transform style attributes in…

68cd565

…to an style object

fix(utilities.js): Format string to lowercase before converting to ca…

4522666

…mel case and assert the string is a string

fix(attributes-to-props.js): Remove unappropriate console logging and…

10ff149

… remove double quote from tests

remarkablemark reviewed Nov 26, 2017

View reviewed changes

refactor: Changes to meet projects requirements

3ca6681

remarkablemark approved these changes Nov 27, 2017

View reviewed changes

remarkablemark self-assigned this Nov 27, 2017

remarkablemark merged commit 8f2bbd2 into remarkablemark:master Nov 27, 2017

d-lazarev pushed a commit to d-lazarev/html-react-parser that referenced this pull request Apr 5, 2019

Merge pull request remarkablemark#45 from ianvieira/master

a68282a

Fix component style attribute parser

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix component style attribute parser #45

Fix component style attribute parser #45

ianvieira commented Oct 25, 2017

coveralls commented Oct 25, 2017

remarkablemark commented Oct 26, 2017 •

edited

coveralls commented Nov 23, 2017

ianvieira commented Nov 23, 2017 •

edited

remarkablemark commented Nov 26, 2017

ianvieira commented Nov 26, 2017 •

edited

coveralls commented Nov 26, 2017

coveralls commented Nov 26, 2017

coveralls commented Nov 26, 2017

ianvieira commented Nov 26, 2017

remarkablemark commented Nov 26, 2017

remarkablemark Nov 26, 2017

remarkablemark Nov 26, 2017

remarkablemark Nov 26, 2017

remarkablemark Nov 26, 2017

ianvieira commented Nov 26, 2017 via email

ianvieira commented Nov 27, 2017

coveralls commented Nov 27, 2017

remarkablemark commented Nov 27, 2017 •

edited

ianvieira commented Nov 27, 2017

remarkablemark commented Nov 27, 2017

remarkablemark commented Nov 28, 2017

Fix component style attribute parser #45

Fix component style attribute parser #45

Conversation

ianvieira commented Oct 25, 2017

coveralls commented Oct 25, 2017

remarkablemark commented Oct 26, 2017 • edited

coveralls commented Nov 23, 2017

ianvieira commented Nov 23, 2017 • edited

remarkablemark commented Nov 26, 2017

ianvieira commented Nov 26, 2017 • edited

coveralls commented Nov 26, 2017

coveralls commented Nov 26, 2017

coveralls commented Nov 26, 2017

ianvieira commented Nov 26, 2017

remarkablemark commented Nov 26, 2017

remarkablemark Nov 26, 2017

Choose a reason for hiding this comment

remarkablemark Nov 26, 2017

Choose a reason for hiding this comment

remarkablemark Nov 26, 2017

Choose a reason for hiding this comment

remarkablemark Nov 26, 2017

Choose a reason for hiding this comment

ianvieira commented Nov 26, 2017 via email

ianvieira commented Nov 27, 2017

coveralls commented Nov 27, 2017

remarkablemark commented Nov 27, 2017 • edited

ianvieira commented Nov 27, 2017

remarkablemark commented Nov 27, 2017

remarkablemark commented Nov 28, 2017

remarkablemark commented Oct 26, 2017 •

edited

ianvieira commented Nov 23, 2017 •

edited

ianvieira commented Nov 26, 2017 •

edited

remarkablemark commented Nov 27, 2017 •

edited