-
Notifications
You must be signed in to change notification settings - Fork 819
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
Symbol support #53
Symbol support #53
Conversation
src/index.js
Outdated
@@ -8,6 +8,7 @@ import RedBox from './components/RedBox'; | |||
import View from './components/View'; | |||
import Text from './components/Text'; | |||
import TextStyles from './sharedStyles/TextStyles'; | |||
import Symbol from './symbol'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Symbol
isn't a great name because it shadows the JS primitive. Could we choose something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh shoot, yes. this was a silly thing to do. how about Symbols
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that's fine :-) it can conceptually overlap, as long as it doesn't literally overlap.
However, I'd also say that this doesn't tell me what a "symbol" is in this context, and it's not super clear to me from the OP either :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh shoot! i forgot to reference the original issue that this proposes to resolve! it's for #28, adding automatic sketch symbol generation and references. 0 for 2 on clarity so far 😆 anyway i'll change it to Symbols
. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, so based on the original issue i'd call it a "SketchSymbol", but any name other than "Symbol" is acceptable :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I'd love it if we could get away with not using SketchX
as a prefix.
The exported API can be split into "things that fit into react-primitives" (View, Text, Image etc) and "Sketch-specific concepts" — TextStyles, Artboard etc.
I agree that Symbol
is not the right name for it because of clashes, but what if we just exported the symbolize
or makeSymbol
or whatever HOC rather than Symbol.symbolize
? I guess we also need Symbol.inject
but there's probably a smart way around that 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed to makeSymbol
and injectSymbols
, both imported from the top namespace. The example in the PR description is updated to demonstrate. Happy to change these as many times as needed until it feels right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jongold fwiw, i think the difference here is that "Artboard" isn't a JavaScript-specific concept, but "Symbol" is - and I think that language-specific construct names are always automatically off limits. You shouldn't have a "Map" either - a "GoogleMap" or a "StreetMap" or something, but not a "Map" - that's taken by the language.
Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
Handle string only errors in RedBox.js
* Support for opacity style * Add test for opacity * Remove TODO comment for opacity
…p into symbol_support
@jongold ready for your review! you can find example usage in |
@lukewestby !!!!!!!!!!!! woo, I'll take a look :) |
@lukewestby awesome work! I'm hitting an error when try to run the symbol example:
Env: Steps to reproduce:
Stack Trace, Click to Expand
➜ symbols git:(symbol_support) ✗ npm run build
Thanks! |
@michaelcarter-wf this is a bug with skpm's webpack config afaik — to fix it locally until we resolve it, |
So sorry this took so long to merge in — this is really wonderful work. I'm going to merge it ASAP and then we can do another PR to add docs you: a+++++ 😍 |
@lukewestby i pulled down this work and it looks awesome, but i noticed that any updates to the symbols essentially breaks their past use, since sketch thinks the update to the symbols makes them new symbols. any way to hydrate the symbols so any use of the symbols get updated to the latest version of the symbols from code? thanks! |
@tehOPEologist — I don't follow - any chance you could post a quick screencast of the behavior? (you can use kap if you don't have a screencasting thingy installed) |
i'm not near my dev environment to replicate/record right now, but essentially:
i'm wondering if there's a way to stop the last thing from happening, and the symbol that's in use get updated to reflect the changes made. that would make this symbol implementation incredibly useful for iterative changes to symbols. |
it looks like injectSymbols just replaces the layers in the Symbols page, which is why the symbols are "missing" 😢 if there was a way to update symbols in place, i think this feature would become a lot more powerful. |
welp, for some reason flow is bailing on master but wasn't on the branch. I have no idea. will look into it this afternoon, can't publish a new version until that's fixed. |
@tehOPEologist — makes sense, let's circle back to it at a later point :) |
* deals with `Module not found: Error: Can't resolve 'sketch-module-console-polyfill'` in setups where `skpm` is linked into the using project. * related: airbnb/react-sketchapp#53 (comment)
👏 Thanks for everybody involved with this PR. So much awesome here. @jongold now that this is merged, would it be possible to get a new release cut? |
@chrislloyd done — was waiting for #110 to merge, just published |
also if anyone gets comfortable with it, we'd appreciate a contribution to the API Documentation! #112 |
Heya!
There are a few things still missing here but I wanted to get this in front of y'all before going down any paths you wouldn't want. I'll outline some things that are missing or weird still but the gist of the API so far can be explained by this example:
What will happen is a new page called
"Symbols"
will be created if it doesn't already exists and the symbol master generated by the call tomakeSymbol
will be dumped there. Then, each renderedWidgetSymbol
will be a symbol instance pointing to the related master. Names of things are best guess based on the name of the class, and names are currently shared and global, so if you have two things calledWidget
one will be overwritten.See also the example in /examples/symbols
How overrides work
I tried to keep the overrides API as similar to the actual experience in Sketch as possible. There are three overridable elements within a symbol: text, images, and other symbols.
Text
Text overrides currently work by mapping either the name from a
<Text />
element or the actual text content onto the key in the overrides object. So, if a symbol master looks like:Then the override is going to look like
But, if you just use a string in your master like
Then the override will look like
Images
Image overrides work by mapping the name assigned to the image element onto another image url string. In the master:
And to override:
Nested Symbols
Nested symbols are overrided by mapping the name given to the symbol instance nested within the master onto the constructor for a different symbol instance. The constructor must ultimately point to a master with the exact same width and height as the original. For a master that contains:
The override is:
You'll notice here we can provide overrides at the top level that apply to nested symbols within a master. This applies whether the nested symbol instance has been overrided or not. So if we didn't override
NestedSymbolA
withNestedSymbolB
, we could include overrides for the contents ofNestedSymbolA
. This is how Sketch presents overrides for nested symbols in its UI, and internally we need to collect all overrides into one object as well so I chose to represent it as a flattened application in the API as well.Things that are weird
css-layout
wants to see some dimensions on the children of a flex container, so if you just render a symbol like<MyInstance />
you're going to end up with no width and no height. This iscounterintuitive -- Sketch seems to clone the layout of the master to each instance when you first create it.
Things to do
sketchapp-json-flow-types
package again once this PR is merged and published.