search

chrisvxd / story2sketch

Convert Storybook into Sketch symbols 💎
Other
402 stars 32 forks source link

add fix for pseudo elements #48

Closed hipstersmoothie closed 5 years ago

hipstersmoothie commented 5 years ago

html-sketchapp doesn't support pseudo elements https://github.com/brainly/html-sketchapp/issues/20.

A fix for this is proposed by https://github.com/brainly/html-sketchapp/issues/20#issuecomment-343702277 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 commented 5 years ago

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 notifications@github.com, wrote:

html-sketchapp doesn't support pseudo elements brainly/html-sketchapp#20. A fix for this is proposed by brainly/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: https://github.com/chrisvxd/story2sketch/pull/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 commented 5 years ago

@chrisvxd any chance you could get to this soon?

hipstersmoothie commented 5 years ago

@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!
hipstersmoothie commented 5 years ago

@chrisvxd typo fixed!

chrisvxd commented 5 years ago

@hipstersmoothie 👏 thank you! Merged!

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

chrisvxd commented 5 years ago

@hipstersmoothie just released v1.4.0 with your fixes!

hipstersmoothie commented 5 years ago

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 notifications@github.com 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 https://github.com/chrisvxd/story2sketch/pull/48#issuecomment-443668742, or mute the thread https://github.com/notifications/unsubscribe-auth/ABIyBAh0W0At_GTnmIWDU-9PWNsIkKy6ks5u1QKJgaJpZM4Yn1CS .

chrisvxd commented 5 years ago

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!