Remove n^2 rendering problem with large symbol sets #235
Conversation
|
Thanks, @ianhook – this is SOO much faster! I've added a branch to test this on the demo repo that I created for #230: https://github.com/alampros/react-sketchapp-svg-render-test/tree/test-pull-235 |
examples/symbols/src/my-command.js
Outdated
| @@ -6,54 +6,51 @@ import { | |||
| View, | |||
| Image, | |||
| makeSymbol, | |||
| injectSymbols, | |||
There was a problem hiding this comment.
Can this be removed safely? It's throwing an eslint error.
|
This error is thrown on render if It looks like adding this check (from if (mastersNameRegistry === null) {
getExistingSymbols();
} |
src/symbol.js
Outdated
| if (!notSymbolsPage) { | ||
| notSymbolsPage = globalContext.document.addBlankPage(); | ||
| } | ||
| globalContext.document.setCurrentPage(notSymbolsPage); |
There was a problem hiding this comment.
shouldn't we store the current page at the beginning of the injectSymbols method and go back to it instead of getting the first one here?
|
What's involved in getting this merged and released? |
| @@ -28,6 +28,5 @@ module.exports = { | |||
| View, | |||
| Platform, | |||
| makeSymbol, | |||
| injectSymbols, | |||
There was a problem hiding this comment.
this is a breaking change. Could we maybe expose it again? (it's actually useful if you just want to generate the symbols without rendering anything as in #172 )
|
merged as part of #254 |
The original symbol code required
injectSymbolsto be called separately frommakeSymbolsbecauseinjectSymbolsdeletes the Symbols page and recreates it on every call. This can take a while as seen in #230.I put the
injectSymbolscall in the render method, because this is the only time it needs to be called.