ES5 build, recursive node tree (#78), TypeScript typings, main module entry point, bugfixing #83

TheReincarnator · 2018-04-17T11:48:18Z

No description provided.

TheReincarnator · 2018-04-17T11:51:26Z

Oh, I have some linting to do :-)

TheReincarnator · 2018-04-17T12:27:01Z

Ok, Linter and Unit tests are happy now, but the E2E tests fail. I still have some homework to do :-)

kdzwinel · 2018-04-17T13:21:55Z

.editorconfig

+insert_final_newline = true
+spaces_around_brackets = false
+spaces_around_operators = false
+trim_trailing_whitespace = true


as discussed - this file is automatically copied form https://github.com/brainly/frontend-tools-configs so we don't want it in the repo

kdzwinel · 2018-04-17T13:27:46Z

package.json

  },
  "dependencies": {
    "murmur2js": "^1.0.0",
    "normalize-css-color": "^1.0.2",
    "sketch-constants": "^1.1.0",
    "sketchapp-json-plugin": "^0.1.2"
  },
+  "files": ["asketch2sketch", "html2asketch", "build"],


let's exclue build/asketch2sketch.sketchplugin

kdzwinel · 2018-04-17T13:34:45Z

html2asketch/helpers/bcr.js

+    right: groupBCR.right,
+    bottom: groupBCR.bottom,
+    width: groupBCR.right - groupBCR.left,
+    height: groupBCR.bottom - groupBCR.top


use already calculated width and height

Actually, the groupBCR does not have width and height properties, as the reduction produces top,left,bottom,right, only. So for the final result, those four properties are needed.

You are right 👍

kdzwinel · 2018-04-17T13:36:24Z

html2asketch/nodeToSketchLayers.js

-        width: textBCR.width,
-        height: textBCR.height,
+        x: textBCR.left || 0,
+        y: (textBCR.top || 0) + fixY,


let's skip || 0

kdzwinel · 2018-04-17T13:40:57Z

html2asketch/nodeToSketchLayers.js

@@ -181,8 +184,6 @@ export default function nodeToSketchLayers(node, options) {
      }
    }

-    style.addOpacity(opacity);


let's put that into options and make enabled by default

While trying to fix this I was wondering if the former implementation was even correct.
Currently, the opacity is set to the shape-group only, but not to any other layer, like text or SVG. So we get a transparent background and an opaque text on it.
I think we should either add the opacity to all layers, or remove it entirely, documenting that it is the container's responsibility to set the opacity.
What do you think?

Good point - it should be added to all layers, but feel free to skip this fact for the sake of this PR, I can handle it separately.

I continued to think about this layers vs. group opacity thing, and I come to the conclusion that the problem is even worse. When adding opacity to each layer, the (outside coded) recursion will have to apply parent opacity to children as well, as the node children inside transparent parents will not issue any opacity value.
I think it is a better idea to leave out the opacity entirely in regards of the layers, and let the caller apply opacity to the container. Any other approach results in a complex API, as far as I can see.
Or do you have a good idea as alternative?

Hm, not sure if I fully understand.

For nodeToSketchLayers we should add opacity to all produced layers (with an option to turn opacity off for nodeTreeToSketchGroup)

For nodeTreeToSketchGroup we should set opacity on groups and not on the layers

I don't see where the problem is 🤔

Yes, that was the approach we discussed in the call. My issue is that I can hardly find an intact use-case for the default option with opacity layers.

As long as your DOM node does not have any children, everything is fine. The shape group and text nodes get the opacity, and it looks great.
But what if the DOM node has children (resp. parents have opacity)?

An example:
We have two nested nodes, A and B (B inside A).
A has an opacity of 0.5, while B is opaque (no transparency).

The caller will be responsible for a recursion, and for A, nodeToSketchLayers will return layers with opacity 0.5, while for B, it will return opaque layers.
Now the caller wants to put all layers into one Sketch page.
If they flatten the layers, the node B layers will be opaque, which is wrong, as they are nested in the A node without opacity 0.5.
If he groups the nodes' layers hierarchically and puts the opacity into the grouping, they fail as well, as the node A layers' opacities are now in the way.

So basically my point is that setting a layer opacity does not work at all, unless the nodeToLayers function gets concise of its parents' opacity situation (which multipies up in the entire ancestry). This would have to be an API option, and I think this is not very clean.

Should we have a call? :-)

As discussed in a call, we currently have three options:

the current behavior, where only the current node's opacity is set into the layers

not setting any layer opacity at all, which is required for the grouping function

a solution where nodeToSketchLayers collects the parents' opacities and multiplies them, so it is responsible for the parent opacity situation.

For this PR, I will implement options 1 and 2 (with 1 being the default).
In the future, the 3rd option is the best one when not grouping and should be available as well (or maybe even be a fix for option 1).

👍 I like the idea of making 3rd option a fix for option 1, so I went ahead and opened an issue for it - #84

kdzwinel · 2018-04-17T13:41:59Z

html2asketch/nodeToSketchLayers.js

@@ -195,8 +196,13 @@ export default function nodeToSketchLayers(node, options) {

    const rectangle = new Rectange({width, height, cornerRadius});

+    if (options && isNotEmpty(options.rectangleName)) {
+      leaf.setName(options.rectangleName);


let's make it a callback (node) => string

kdzwinel · 2018-04-17T13:43:43Z

html2asketch/nodeTreeToSketchGroup.js

+
+function isNotEmpty(value) {
+  return value !== undefined && value !== null && value !== '';
+}


let's put this guy into helpers

kdzwinel · 2018-04-17T13:46:12Z

html2asketch/nodeTreeToSketchGroup.js

+
+  // Collect layers for the node level itself
+  const layers = nodeToSketchLayers(node, options) || [];
+


let's check here if node we are trying to process is visible to avoid empty groups

kdzwinel · 2018-04-17T13:46:53Z

html2asketch/nodeTreeToSketchGroup.js

+
+  // Set the group name to the node name, but support a hint in the DOM
+
+  const nodeName = node.getAttribute && node.getAttribute('data-sketch-name');


let's make it more general by having a callback

kdzwinel · 2018-04-17T13:50:09Z

html2asketch/group.d.ts

+
+export default class Group extends Base {
+  constructor(props: GroupProps);
+}


let's see if we can validate these types - see if they match the classes they are discribing

calebdwilliams · 2018-04-17T15:11:50Z

html2asketch/nodeTreeToSketchGroup.js

+}
+
+export default function nodeTreeToSketchGroup(node, options) {
+  const bcr = node.getBoundingClientRect();


If an element is display: contents this will always return with all 0s. For this particular feature, you might consider a check to see if that's the case then check the contents or the HTMLElement.shadowRoot to see if you can't ensure there's an element.

Oh, nice, I didn't know that! Fantastic that you figured it all out 👏 This PR is already massive though, so maybe lets add shadow DOM support in a separate one. WDYT?

@calebdwilliams Thanks for the feedback, I am quite new to frontend technologies, at least the modern ones, so I don't know what the contents display type is, and I only heard of the shadow DOM so far, never used it. Can you please provide an example snippet of the function you have in mind?

TheReincarnator · 2018-04-18T12:42:56Z

By the way, I had to add two more things to the PR in the meantime, I hope you don't mind:

The Artboard model, which is (as my designers say) the standard element to add to a page before adding content to it. The page-creation function now returns a page with an artboard inside.
There was a bug in page.js, where width and height were mixed up. I have put this into a separate commit.

kdzwinel · 2018-04-18T12:53:38Z

@TheReincarnator

bug fix - good catch 👏
adding artboard model - yes! I also wanted to add that 👏
putting an artboard always inside of a page 🤔That's definitely not required - you can have multiple artboards on page or have no artboard at all.

WDYT about making it optional?

Also, WDYT about adding an E2E test for nodeTreeToSketchGroup? We can run it against same test HTML page.

BTW Any news about type validation?

…l-sketchapp#78)

TheReincarnator · 2018-04-18T13:22:07Z

Yes, I will make the artboard optional, good point. I don't know Sketch too well so I definitely need advices here. But an option is good for both sides.
Regarding the E2E test: I will give it a try, never did before :-)
Regarding the type validation: Our wise guys never came to that situation, we develop TypeScript in the first place, so we generate types and do not hand-code them. As of now, I have no solution by hand, sorry.

TheReincarnator · 2018-04-18T13:30:42Z

@kdzwinel The E2E again: Can you give me a hint where you would put the second test? Currently, you have one webpacked inject.js, with a default function that returns the JSON, and one expected.asketch.json. Should I duplicate these for the group and/or page sketch test? Where is the best place to split into separate tests?

TheReincarnator · 2018-04-18T13:31:37Z

Otherwise, I have now reworked everything we discussed, from yesterday and today, all tests are green, and in our project, the result is good. From my point of view, the PR is complete, but I again changed a lot, so please review once again. Thanks!

kdzwinel · 2018-04-18T13:32:19Z

Regarding the E2E test: I will give it a try, never did before :-)

It boils down to this:

add a new method to https://github.com/brainly/html-sketchapp/blob/master/test/inject.js#L8 that calls nodeTreeToSketchGroup on a given node and returns JSON
run it in the headless browser on the test page - https://github.com/brainly/html-sketchapp/blob/master/test/run.js#L56
create another https://github.com/brainly/html-sketchapp/blob/master/test/expected.asketch.json file (expected-groups.asketch.json ?)
compare what you'll get from the headless browser with your new expected-groups.asketch.json file ( https://github.com/brainly/html-sketchapp/blob/master/test/run.js#L67 )

(BTW there is https://github.com/brainly/html-sketchapp/blob/master/test/run.js#L10 to accomodate for randomness in the produced file)

Let me know if you'll run into any issues!

I have no solution by hand, sorry.

No worries, we can handle that separately 👌 #85

kdzwinel · 2018-04-18T13:35:11Z

Everything looks great - fantastic work 👏 I'll give it a try locally tomorrow and merge it 💪

TheReincarnator · 2018-04-18T14:06:20Z

@kdzwinel I have now developed an E2E test by introducing a forEach for both tests like this:

const tests = ['layers', 'page'];

  let anyError = false;

  tests.forEach(async test => {
    const expectedPath = `./expected-${test}.asketch.json`;
    const actualJSON = await page.evaluate(`body2sketch.${test}JSON(document.body)`);

    removeRandomness(actualJSON);

    // update file with expected output by uncommenting the two lines below
    // fs.writeFileSync(path.resolve(__dirname, expectedPath), JSON.stringify(actualJSON, null, 2));
    // if ('dummy test to make linter happy'.length) {
    //   return;
    // }

    const expectedJSONBuffer = fs.readFileSync(path.resolve(__dirname, expectedPath));
    const expectedJSON = JSON.parse(expectedJSONBuffer.toString());

    const diff = jsdiff(expectedJSON, actualJSON);

    if (diff.changed) {
      console.error(`E2E test "${test}": ❌ Oh no! That's not the expected output. See the diff below:`);
      console.log(diff.text);
      anyError = true;
    }
  });

  browser.close();

  if (anyError) {
    process.exit(1);
  } else {
    console.log('E2E tests: ✅');
  }

I decided to test nodeTreeToSketchPage, with artboard, so the test covers all the functionality.
What do you think?

~~Unfortunatelly, I cannot run the JSON generation because of npm i issues I have. So I can provide the commit without the JSON (and without having tested it), maybe you can finish this?~~

Using yarn, I have now managed to install. And the tests fail, I will fix this :-)

TheReincarnator · 2018-04-19T07:26:37Z

@kdzwinel I am so proud, my first puppeteer E2E test :-)
I finally managed to get the problems fixed, and here we go, another commit for E2E-testing the nodeTreeToSketchPage (also covering nodeTreeToSketchGroup).
Feedback welcome, from my point of view everything is done now for this PR.

kdzwinel · 2018-04-19T20:53:15Z

I am so proud, my first puppeteer E2E test :-)

You rock 🎸

OK Tom, I'm merging this because it's a pure gold 🥇I did find some small issues, but I'll either fix them myself on master or create tickets for them. Thanks again for a great contribution 👏

TheReincarnator force-pushed the grouping-and-fonts branch 3 times, most recently from 9c53b86 to 259a338 Compare April 17, 2018 12:20

TheReincarnator force-pushed the grouping-and-fonts branch 3 times, most recently from e4b3f23 to 3bb179b Compare April 17, 2018 12:55

kdzwinel reviewed Apr 17, 2018

View reviewed changes

calebdwilliams reviewed Apr 17, 2018

View reviewed changes

TheReincarnator added 5 commits April 18, 2018 11:35

Bump version

8d9bd8d

Add Babel ES5 compilation (e.g. for Electron)

a6b4346

Add vscode-config to align with editor-config

962c2c6

Add option to skip system fonts

5982223

Fix position calculation (bounding rectangle) for older DOM API

f4992d2

TheReincarnator force-pushed the grouping-and-fonts branch 2 times, most recently from 1cbfece to abcec5b Compare April 18, 2018 12:12

kdzwinel mentioned this pull request Apr 18, 2018

Opacity should be applied to all layers #84

Open

TheReincarnator force-pushed the grouping-and-fonts branch from abcec5b to 979d821 Compare April 18, 2018 12:38

TheReincarnator force-pushed the grouping-and-fonts branch from 979d821 to 38e98ad Compare April 18, 2018 12:44

TheReincarnator added 4 commits April 18, 2018 15:13

Add conversion of node hierarchies into Sketch group hierarchies (htm…

be4f3ae

…l-sketchapp#78)

Fix swapped width/height properties in Page

7c66559

Add TypeScript typings

758e80f

Add index.js as entry point, move model to subfolder

1bc6302

TheReincarnator force-pushed the grouping-and-fonts branch from 38e98ad to 1bc6302 Compare April 18, 2018 13:15

Add E2E test for nodeTreeToSketchPage

fe9c20f

kdzwinel approved these changes Apr 19, 2018

View reviewed changes

kdzwinel merged commit 8e4dfe4 into html-sketchapp:master Apr 19, 2018

This was referenced Apr 19, 2018

Modify x, y to left, top in getBoundingClientRect() #51

Closed

Make nodeToSketchLayers recursive #78

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ES5 build, recursive node tree (#78), TypeScript typings, main module entry point, bugfixing #83

ES5 build, recursive node tree (#78), TypeScript typings, main module entry point, bugfixing #83

TheReincarnator commented Apr 17, 2018

TheReincarnator commented Apr 17, 2018

TheReincarnator commented Apr 17, 2018

kdzwinel Apr 17, 2018

kdzwinel Apr 17, 2018

kdzwinel Apr 17, 2018

TheReincarnator Apr 18, 2018

kdzwinel Apr 18, 2018

kdzwinel Apr 17, 2018

kdzwinel Apr 17, 2018

TheReincarnator Apr 17, 2018

kdzwinel Apr 17, 2018 •

edited

TheReincarnator Apr 18, 2018 •

edited

kdzwinel Apr 18, 2018

TheReincarnator Apr 18, 2018 •

edited

TheReincarnator Apr 18, 2018 •

edited

kdzwinel Apr 18, 2018

kdzwinel Apr 17, 2018

kdzwinel Apr 17, 2018

kdzwinel Apr 17, 2018

kdzwinel Apr 17, 2018

kdzwinel Apr 17, 2018

calebdwilliams Apr 17, 2018

kdzwinel Apr 17, 2018

TheReincarnator Apr 17, 2018

TheReincarnator commented Apr 18, 2018

kdzwinel commented Apr 18, 2018 •

edited

TheReincarnator commented Apr 18, 2018 •

edited

TheReincarnator commented Apr 18, 2018

TheReincarnator commented Apr 18, 2018

kdzwinel commented Apr 18, 2018 •

edited

kdzwinel commented Apr 18, 2018

TheReincarnator commented Apr 18, 2018 •

edited

TheReincarnator commented Apr 19, 2018

kdzwinel commented Apr 19, 2018


		// Collect layers for the node level itself
		const layers = nodeToSketchLayers(node, options) \|\| [];


		// Set the group name to the node name, but support a hint in the DOM

		const nodeName = node.getAttribute && node.getAttribute('data-sketch-name');

ES5 build, recursive node tree (#78), TypeScript typings, main module entry point, bugfixing #83

ES5 build, recursive node tree (#78), TypeScript typings, main module entry point, bugfixing #83

Conversation

TheReincarnator commented Apr 17, 2018

TheReincarnator commented Apr 17, 2018

TheReincarnator commented Apr 17, 2018

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

Choose a reason for hiding this comment

kdzwinel Apr 17, 2018 • edited

Choose a reason for hiding this comment

TheReincarnator Apr 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheReincarnator Apr 18, 2018 • edited

Choose a reason for hiding this comment

TheReincarnator Apr 18, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheReincarnator commented Apr 18, 2018

kdzwinel commented Apr 18, 2018 • edited

TheReincarnator commented Apr 18, 2018 • edited

TheReincarnator commented Apr 18, 2018

TheReincarnator commented Apr 18, 2018

kdzwinel commented Apr 18, 2018 • edited

kdzwinel commented Apr 18, 2018

TheReincarnator commented Apr 18, 2018 • edited

TheReincarnator commented Apr 19, 2018

kdzwinel commented Apr 19, 2018

kdzwinel Apr 17, 2018 •

edited

TheReincarnator Apr 18, 2018 •

edited

TheReincarnator Apr 18, 2018 •

edited

TheReincarnator Apr 18, 2018 •

edited

kdzwinel commented Apr 18, 2018 •

edited

TheReincarnator commented Apr 18, 2018 •

edited

kdzwinel commented Apr 18, 2018 •

edited

TheReincarnator commented Apr 18, 2018 •

edited