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

Use react-sketchapp directly? #37

Open
arahansen opened this issue Dec 2, 2017 · 18 comments
Open

Use react-sketchapp directly? #37

arahansen opened this issue Dec 2, 2017 · 18 comments

Comments

@arahansen
Copy link
Collaborator

So in the README, html-sketchapp mentions that they pulled heavily from the source of react-sketchapp to implement this tool. Looking through the source of html-sketchapp, it seems a lot of this seems to be related to how sketch json is constructed.

I'm wondering the reason why this code was copied over, rather than directly leveraging the work react-sketchapp has done to generate sketch elements.

For example a potential updated workflow could look something like this:

  1. Run a simplified version of nodeToSketchLayer that grabs the styling and layout information for a given element
  2. Instead of generating sketch json (using the Rectangle, ShapeGroup, Text classes etc), html-sketcahpp generates react-sketchapp components which handles the sketch json generation for us.
  3. Run react-sketchapp as a plugin instead of the custom plugin in html-sketchapp

The benefits here is that html-sketchapp could more directly leverage the development happening in react-sketchapp, and instead of developing similar solutions in parallel, we can build a solution that works for both libraries, and maintains a more cohesive ecosystem.

I've been messing around a bit with what an implementation of this might look like. And I'd be happy to share my findings there. But before I continue down this path much further, I would like to have a discussion and understand the history of "forking" react-sketchapp's implementation rather than taking a dependency on the library directly.

@arahansen
Copy link
Collaborator Author

I also just came across this Pr: #36

If we were able to figure out a way that incorporates what I've proposed, these "syncs" wouldn't need to happen.

@kdzwinel
Copy link
Collaborator

kdzwinel commented Dec 2, 2017

That sounds really reasonable to me! It would simplify the project and let us focus on getting data from the browser. We would also get things like SVG support (open PR on react-sketchapp) for free.

generates react-sketchapp components

We are talking here about generating jsx code? Or is there any intermediate representation? Generating jsx code seems a bit hackish, but not absolutely crazy. What we will actually get is "html ➡️ generic react components" generator, which might be useful also for other things than "html ➡️ sketch" 🤔

BTW I created a small jsxgenerator that takes React component objects and serializes them as jsx, maybe it will be useful here.

the history of "forking" react-sketchapp's implementation rather than taking a dependency on the library directly

I started w/o looking into react-sketchapp code and hoping that I'll be able to generate valid Sketch files in the browser using JS only. Only when I got to text and images I realized that my initial plan won't work. I thought about importing react-sketchapp as a library, but it turned out to be a bit more complicated than I hoped for (e.g. code isn't really structured to be used as a library + they use flow so I'd have to change my build process). It didn't occur to me at that time that react-sketchapp can be my target.

@kdzwinel
Copy link
Collaborator

kdzwinel commented Dec 2, 2017

Maybe instead of generating jsx code it'd make sense to generate sth like TreeNode's directly? Generating jsx code only so that react-sketchapp can parse it to get back position/styling information seems wasteful. On the other hand, as I mentioned before, generating react components out of html might turn out to be useful in other projects.

@kdzwinel
Copy link
Collaborator

kdzwinel commented Dec 2, 2017

@markdalgleish contributed to both projects, so he may have some useful insight.

@jeroenransijn @KimDal-hyeong I'd also love to hear your thoughts!

@markdalgleish
Copy link
Contributor

Sorry, I haven't actually contributed to react-sketchapp, only this project, so I don't have a clear idea of the overlap between the projects internally.

My first thought is that it might make more sense to figure out a common lower level library that could be shared between html-sketchapp and react-sketchapp. @jongold?

@jemgold
Copy link

jemgold commented Dec 2, 2017 via email

@arahansen
Copy link
Collaborator Author

I can certainly start investigating what that might look like!

I've briefly looked into the react-sketchapp source before, and am more familiar with the html-sketcchapp source. But I'd love to learn more :)

@markdalgleish
Copy link
Contributor

@kdzwinel lol, sorry, you're right—totally forgot I contributed a couple of minor commits to react-sketchapp: https://github.com/airbnb/react-sketchapp/commits?author=markdalgleish - not enough to help answer this question properly, though.

@arahansen
Copy link
Collaborator Author

I still think there could be value to consuming react-sketchapp directly, since I believe there is some duplication in how the actual sketch plugin is being built. They seem to me to be doing very similar things: converting json and assets into sketch elements.

@kdzwinel hit on a good point that we are essentially generating react components from html and related styles (which does seem like a useful utility in itself). Instead of writing react-sketchapp components by hand (or with React-primitives) we are generating the definition of the components based on what we can compute from nodeToSketchLayers.js

@markdalgleish
Copy link
Contributor

My biggest concern with this approach is that we'd be reaching into library internals, so it wouldn't be particularly stable, and semantic versioning goes out the window.

@arahansen
Copy link
Collaborator Author

@markdalgleish which internals would we need? I was thinking we would be able to get away with

import { Text, Img, View, Artboard } from 'react-sketchapp' etc

I totally concede I could be missing something here, which is why I opened this issue to discuss further :)

@markdalgleish
Copy link
Contributor

Me too :) I might have the wrong idea, but it seemed like what's needed here wasn't part of the public API. Sounds like I might be wrong, though.

@arahansen
Copy link
Collaborator Author

For those interested, I've opened a WIP PR that uses react-sketchapp to render to sketch. Here: #39

Would love to get some feedback on the direction and hear what you all think

@kdzwinel
Copy link
Collaborator

Sorry I didn't manage to look at the next yet - too many things were happening at the same time 😵 I hope to charge up over the next week and get back to it.

Have a great holidays everyone !! ❄️

@jeroenransijn
Copy link
Contributor

Chiming in pretty late here. I am all for piggy backing of react-sketchapp so we don't have to solve things in two places. What is the progress currently on this?

@kdzwinel
Copy link
Collaborator

kdzwinel commented Mar 2, 2018

I spent some time today working on the 'next' branch. It looks pretty good, but it also looks like we will lose some functionality (e.g. gradients) because react-sketchapp doesn't support them. So that put me off a bit, but I hope that things like SVG support will make up for it.

Anyway, I started experimenting with replacing intermediate representation ("asketch.json") with something custom that makes more sense in connection with react-sketchapp. So far I have this:

{
  colors: ['#rgb', ],
  textStyles: [TEXT, ],
  pages: [{
     name: '',
     items: [{
        name: '',
        symbol: true,
        component: VIEW/TEXT/IMAGE
     }, ]
  }, ]
}

And those TEXT/VIEW/IMAGE would look something like this:

{
  type: 'view',
  name: ''
  styles: {backgroundColor: '#000',},
  frame: {left: 0, top: 10, width: 500,},
  children: [
     {
        type: 'image',
        content: 'https://bla.bla/img.png',
        frame: {left: 0, top: 10, width: 500,}
     },
     {
        type: 'text',
        content: 'some text',
        styles: {fontFamily: 'Arial',},
        frame: {left: 0, top: 10, width: 500,}
     },
     {
        type: 'view',}
  ]
}

So a tree structure of react primitives in a JSON form.

As I see it, new representation will have following benefits over existing one:

  • everything in a single file (instead of document.asketch.json and page.asketch.json)
  • more readable (no cryptic Sketch props)
  • tree structure will be maintained (it should help us get overflows working - assuming that react-sketchapp supports them)
  • easy to convert into representation expected by react-sketchapp (e.g. snippet above would be translated to: <View> <Image /><Text /><View /> </View>)

I might be missing something and this format will probably change a bit while I'm implementing it, but that's what I'm aiming for right now. It's still an open discussion though - if anyone has a better intermediate structure in mind - let me know. Also, if anyone sees some bigger opportunity (e.g. getting rid of intermediate state completely) please share - I'm having a hard time wrapping my mind around all of this.

@jemgold
Copy link

jemgold commented Mar 2, 2018 via email

@kdzwinel
Copy link
Collaborator

kdzwinel commented Mar 5, 2018

Oh, I didn't realize that react primitives are the bottleneck here. Both css and sketch support gradients, so I'd be great for react-sketchapp to also support them. I can look into that when I finish html-sketchapp integration with react-sketchapp. ATM I'd doing a slight detour and creating a helper library that transforms DOM nodes into react primitives - https://github.com/brainly/html-react-primitives .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants