Rework render flow. #297

jaridmargolin · 2018-04-21T20:14:56Z

[Edited: 2018/05/07]

Breaking Changes (kind of)

Now throws if the "Symbols" page is explicitly passed in as the container on the render method. Previously if you explicitly passed in the "Symbols" pages as a container, it would create a new page and render onto that. I would consider that to be unexpected behavior.
Now throws an error if you attempt to render a Document component into a node intended to be a child of Document. Previous behavior was unexpected. It would result in reseting the currently selected page only, and then it would additionally render all child pages defined within the passed Document. This would essentially result in pages indefinitely being added to the document upon calling render. This may possibly close Multiple pages are created when using makeSymbol after saving #262.

Enhancements

More predictable rendering of RedBox. The previous implementation would not respect the passed in container. It would always render the RedBox into the currently selected page. This could have real negative consequences, by accidentally rendering over pages that were manually constructed. The new behavior will first check for a passed in container, if it does not exist, it will either render onto the currently selected page, or it will create a new page if the currently selected page is the "Symbols" page.
Adds support for rendering a Page component into a container passed through the render method. This allows for rendering multiple Artboards onto an existing page. This would close Add Canvas component (enables rendering multiple Artboard's on existing page) #291 as there would no longer be a need for the Canvas component.

Additional Notes

As mentioned in Remove defaultProps from Page component (enables auto-incrementing page name) #296: This is a larger concern than just this particular PR, but I don't see where or how I can create tests to confirm functionality that involves Sketch's behavior
I altered a few pieces of puzzling code. Because the render code path does not have automated tests, it is hard to confirm if this will result in any regressions. I have left inline comments on Github in places where I was uncertain.

jaridmargolin · 2018-04-21T20:21:47Z

src/render.js

-    findPageData(children[i], depth + 1);
-  }
-  return accumulated;
-};


It is unclear to me why this method exists. It appears to have recursive functionality, but the nested call to findPageData doesn't pass through the accumulated array. So really it recursively searches the tree, but only returns top level results.

I have completely removed this method as I see no reason not to simply loop through the children of the Document component.

jaridmargolin · 2018-04-21T20:27:08Z

src/resets.js

-  for (let l = 0; l < layers.count() - 1; l += 1) {
-    const layer = layers.objectAtIndex(l);
-    layer.removeFromParent();
-  }


This was especially puzzling to me.

From a code standpoint, I don't understand Skip last child since it is the container itself... The values of container.children() are only the containers children. It would make no sense for container.children() to contain itself.

When manually running this piece of code, it did exactly what I expected it to... It left a child on the page. To be more clear. If I create a page, I insert and Artboard on that page, and I run restLayer on that page, the Artboard is not removed.

Must be a user error of some sort, but upon further investigation this check does appear necessary when using layer.children(). However, we may be able to bypass this check by instead utilizing layer.layers() which appears to return only the direct descendants of the layer.

mathieudutour

There are some good stuff in there but it should probably be rebased on top of #254 which cleans up a bit how we handle the container. Could you perhaps review that PR so I can merge it?

mathieudutour · 2018-04-22T15:19:34Z

src/render.js

+    const tree = buildTree(element);
+
+    if (tree.type === 'document' && container) {
+      throw Error('Cannot render `Document` component into a container');


I don't think that's right. What if I pass a document as the container? I might not want to render into the current document

It does look like based on the work done in #254 this would be an incorrect assumption on my part.

I'll revisit this after reviewing #254 in more detail (I've only had a second to glance at it).

mathieudutour · 2018-04-22T15:22:34Z

src/render.js

-      });
-    }
+  return tree.children.map((child, i) => {
+    const page =


shall we add a check to make sure the child is a Page?

We can... would you want to throw if it isn't?

Yeah that'd be nice

mathieudutour · 2018-04-22T15:24:01Z

src/render.js

+    // assume if name is set on this nested page, the intent is to overwrite
+    // the name of the page it is getting rendered into
+    if (tree.props.name) {
+      container.setName(tree.props.name);


jaridmargolin · 2018-04-24T14:00:34Z

Yep. After reviewing #254 it is clear that this work should be rebased ontop of it. I will wait for it's merge and then push appropriate updates. Thanks for the set of 👀@mathieudutour

…of entire `Document`

jaridmargolin · 2018-05-07T14:05:48Z

@mathieudutour - I have refreshed this PR of off current master (which now includes #254). When you have a moment, could you please give this a look?

mathieudutour

I'm wondering if we really need to forbid rendering into the Symbols page. There is nothing special about in Sketch except being the default location where symbols are going but they can be created in any page and you can any type of layers to the Symbols page

jaridmargolin · 2018-05-07T15:03:48Z

I'm wondering if we really need to forbid rendering into the Symbols page. There is nothing special about in Sketch except being the default location where symbols are going but they can be created in any page and you can any type of layers to the Symbols page

I more or less agree with your sentiment, but I think that is a larger question that would need to be addressed in a standalone task. The current implementation of symbols makes a few assumptions about the Symbols page.

mathieudutour

sounds good. Can you create the follow up issue so that we don't forget?

jaridmargolin commented Apr 21, 2018

View reviewed changes

jaridmargolin force-pushed the multiple-artboards branch 3 times, most recently from 9f811b8 to aa3dfcd Compare April 21, 2018 21:01

mathieudutour requested changes Apr 22, 2018

View reviewed changes

Add isNativeSymbolsPage util

6d1ee33

jaridmargolin force-pushed the multiple-artboards branch from 3952049 to e55be5f Compare May 7, 2018 13:41

Add support for rending Page component(s) without requiring render …

5aa564c

…of entire `Document`

jaridmargolin force-pushed the multiple-artboards branch from e55be5f to 5aa564c Compare May 7, 2018 13:56

mathieudutour reviewed May 7, 2018

View reviewed changes

mathieudutour approved these changes May 7, 2018

View reviewed changes

jaridmargolin merged commit a808e5b into airbnb:master May 7, 2018

jaridmargolin deleted the multiple-artboards branch May 7, 2018 15:30

jaridmargolin mentioned this pull request May 7, 2018

Investigate removing "special" meaning of symbols page #316

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework render flow. #297

Rework render flow. #297

jaridmargolin commented Apr 21, 2018 •

edited

Loading

jaridmargolin Apr 21, 2018

jaridmargolin Apr 21, 2018 •

edited

Loading

jaridmargolin Apr 24, 2018

mathieudutour left a comment

mathieudutour Apr 22, 2018

jaridmargolin Apr 23, 2018

mathieudutour Apr 22, 2018

jaridmargolin Apr 23, 2018

mathieudutour Apr 23, 2018

mathieudutour Apr 22, 2018

jaridmargolin commented Apr 24, 2018

jaridmargolin commented May 7, 2018

mathieudutour left a comment

jaridmargolin commented May 7, 2018

mathieudutour left a comment

Rework render flow. #297

Rework render flow. #297

Conversation

jaridmargolin commented Apr 21, 2018 • edited Loading

Breaking Changes (kind of)

Enhancements

Additional Notes

Choose a reason for hiding this comment

jaridmargolin Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathieudutour left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaridmargolin commented Apr 24, 2018

jaridmargolin commented May 7, 2018

mathieudutour left a comment

Choose a reason for hiding this comment

jaridmargolin commented May 7, 2018

mathieudutour left a comment

Choose a reason for hiding this comment

jaridmargolin commented Apr 21, 2018 •

edited

Loading

jaridmargolin Apr 21, 2018 •

edited

Loading