add fix for pseudo elements

[hipstersmoothie]

html-sketchapp doesn't support pseudo elements html-sketchapp/html-sketchapp#20.

A fix for this is proposed by html-sketchapp/html-sketchapp#20 (comment) but the maintainer doesn't want to add it due to it not covering all cases.

I think this would be a good place for the code to live though. I have test it with the icon font that my app uses and it works pretty well. Definitely better than not rendering at all.

Currently it runs by default for every page. This behavior could be guarded by a flag though. I am willing to add this if you want @chrisvxd. Are you open to adding this?

[chrisvxd]

Thanks for this Andrew! I’m currently out of town but will review in the coming days.

…

On 18 Nov 2018, 08:02 +0000, Andrew Lisowski ***@***.***>, wrote: html-sketchapp doesn't support pseudo elements html-sketchapp/html-sketchapp#20. A fix for this is proposed by html-sketchapp/html-sketchapp#20 (comment) but the maintainer doesn't want to add it due to it not covering all cases. I think this would be a good place for the code to live though. I have test it with the icon font that my app uses and it works pretty well. Definitely better than not rendering at all. Currently it runs by default for every page. This behavior could be guarded by a flag though. I am willing to add this if you want @chrisvxd. Are you open to adding this? You can view, comment on, or merge this pull request online at: #48 Commit Summary • add fix for pseudo elements File Changes • M src/browser/page2layers.js (52) Patch Links: • https://github.com/chrisvxd/story2sketch/pull/48.patch • https://github.com/chrisvxd/story2sketch/pull/48.diff — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[hipstersmoothie]

@chrisvxd any chance you could get to this soon?

[chrisvxd]

Yup, sorry about the delay. Busy time of year.

First off, thanks for the contribution! 👏

I've spoken to the Konrad from the html-sketchapp team about this PR, and I think despite it not being supported upstream, I'm happy to bring it in (along with other tricks like it) until they are supported properly upstream.

My review:

1. Error

I've not been able to get this working as the code seems to throw an error when I run it against the official storybook example:

yarn compile
./lib/server/index.js --verbose --output test.asketch.json --concurrency 4 --url https://storybooks-official.netlify.com/iframe.html

Produces the error:

Error: Evaluation failed: TypeError: allElements[i].className.replace is not a function
    at fixPseudoElements (/Users/chrisvilla/Projects/story2sketch/lib/server/../browser/page2layers.bundle.js:2069:59)
    at Object.getSymbol (/Users/chrisvilla/Projects/story2sketch/lib/server/../browser/page2layers.bundle.js:2119:3)
    at <anonymous>:4:12
    at ExecutionContext.evaluateHandle (/Users/chrisvilla/Projects/story2sketch/node_modules/puppeteer/lib/ExecutionContext.js:58:15)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:182:7)

Along with a load of other errors because the page is no longer defined (that's a problem on my side, not yours, see #51).

This occurs when testing against my chris/add-examples branch example too. Master runs cleanly against both of these (although fails to produce output on the official storybook example).

2. Tests

This is a more significant problem than this PR, but story2sketch really needs tests (#52). I feel a bit nervous about letting in something that actually modifies the DOM into story2sketch without test coverage, but unfortunately I'm pretty time constrained and haven't had the time to set up the testing for this project. If I do merge this as-is, I might be tempted to hold off doing a release until some test coverage is added to story2sketch.

3. Format

Please execute prettier on the code - I need to add some linting (and will do so as part of #52).

[hipstersmoothie]

@chrisvxd

1. Fixed this. This was an issue where SVG elements dont actually use a string for the className (The script passed but i could not open it in sketch, although i could no open it on master either)
2. I have hidden this code behind the --fixPseudo flag. This behavior shouldn't effect regular users and is opt in. This should mitigate the risk quite a bit
3. Done!

[chrisvxd]

Great idea re the flag! Have managed to test this and it's working as expected.

One small typo and will merge 😎

[chrisvxd]

I love it when people add docs! However, small typo: "elemenets"

[hipstersmoothie]

@chrisvxd typo fixed!

[chrisvxd]

@hipstersmoothie 👏 thank you! Merged!

NB I squashed into conventional commit format, so you might want to reset your head to master.

[chrisvxd]

@hipstersmoothie just released v1.4.0 with your fixes!

[hipstersmoothie]

Awesome! Thanks a lot. Sorry I didn’t notice the commit format til I was done

…

On Mon, Dec 3, 2018 at 2:52 AM Chris Villa ***@***.***> wrote: @hipstersmoothie <https://github.com/hipstersmoothie> just released v1.4.0 with your fixes! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#48 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABIyBAh0W0At_GTnmIWDU-9PWNsIkKy6ks5u1QKJgaJpZM4Yn1CS> .

[chrisvxd]

No worries about commit format! I tend to squash most PRs that come through at the moment since many people aren't familiar with conventional commits.

It's another place for me to add linting, though!