Updated the public API to no-longer extract nodes #108

2015-08-13T02:06:11-07:00

ColinEberhardt commented

2015-08-13 02:06:11 -07:00

(Migrated from github.com)

When this project was first released the example in the README didn't execute as expected (#24) so an adapter was put in place to recreate this behaviour (#50).

However, on reflection my feeling is that this was a mistake! I think code provided in the README was more illustrative than literal, especially as the early users of this library were clearly not using the JavaScript API.

The current adapted JS API still has issues #61 #62 - I also think that it has some fundamental design flaws.

Currently computeLayout takes a node tree, and optionally 'fills' it out with the required layout / children and style properties. It performs layout, then extracts the layout information into its own tree, which is then returned.

There are a few issues with this approach:

It mutates the node tree that was passed to computeLayout which can be considered a side-effect.
As a user, I now have a tree of layout information, typically I would want to apply this in some fashion to the original node tree, so I now have to traverse two trees side-by-side.

With d3fc we do not use the adapted API due to (2) above.

I propose that the public API should be changed so that it mutates the node tree that is passed to computeLayout, adding the layout information to each node. This should be made clear by having a void function.

I have tested this updates implementation against d3fc and it works for our needs.

fixes #61 fixes #62 fixes #107

When this project was first released the example in the README didn't execute as expected (#24) so an adapter was put in place to recreate this behaviour (#50). However, on reflection my feeling is that this was a mistake! I think code provided in the README was more illustrative than literal, especially as the early users of this library were clearly not using the JavaScript API. The current adapted JS API still has issues #61 #62 - I also think that it has some fundamental design flaws. Currently `computeLayout` takes a node tree, and optionally 'fills' it out with the required layout / children and style properties. It performs layout, then extracts the layout information into its own tree, which is then returned. There are a few issues with this approach: 1. It mutates the node tree that was passed to `computeLayout` which can be considered a side-effect. 2. As a user, I now have a tree of layout information, typically I would want to apply this in some fashion to the original node tree, so I now have to traverse two trees side-by-side. With d3fc we do not use the adapted API due to (2) above. I propose that the public API should be changed so that it mutates the node tree that is passed to `computeLayout`, adding the layout information to each node. This should be made clear by having a void function. I have tested this updates implementation against d3fc and it works for our needs. fixes #61 fixes #62 fixes #107

vjeux commented

2015-08-13 09:25:01 -07:00

(Migrated from github.com)

As a user, I now have a tree of layout information, typically I would want to apply this in some fashion to the original node tree, so I now have to traverse two trees side-by-side.

The design I use for react-native is that there are two trees: the shadow node tree which is completely react-native specific and the layout tree which is the one in css-layout. Because you cannot extend C-struct easily what i'm doing instead is that each layout node has a void* pointer to the corresponding shadow node and a method getChildren(void*) that returns the new set of layout nodes for children.

var s1 = {
  type: 'div',
  layout: new CSSNode(...),
  children: [s2, s3, s4],
  getChildren() { return children.map(child => child.layout); }
};
s1.layout.metadata = s1;

this is indeed a bit convoluted but has the great advantage of letting you manage your tree without impacting the css tree structure

> As a user, I now have a tree of layout information, typically I would want to apply this in some fashion to the original node tree, so I now have to traverse two trees side-by-side. The design I use for react-native is that there are two trees: the shadow node tree which is completely react-native specific and the layout tree which is the one in css-layout. Because you cannot extend C-struct easily what i'm doing instead is that each layout node has a void\* pointer to the corresponding shadow node and a method getChildren(void*) that returns the new set of layout nodes for children. ``` var s1 = { type: 'div', layout: new CSSNode(...), children: [s2, s3, s4], getChildren() { return children.map(child => child.layout); } }; s1.layout.metadata = s1; ``` this is indeed a bit convoluted but has the great advantage of letting you manage your tree without impacting the css tree structure

ColinEberhardt commented

2015-08-13 09:55:34 -07:00

(Migrated from github.com)

The design I use for react-native is that there are two trees:

That's very similar to our approach with d3fc, we have the SVG 'DOM' node tree which we cannot add a layout property to, so we construct a node tree for the purposes of layout:

// creates the structure required by the layout engine
function createNodes(el) {
    function getChildNodes() {
        var children = [];
        for (var i = 0; i < el.childNodes.length; i++) {
            var child = el.childNodes[i];
            if (child.nodeType === 1) {
                if (child.getAttribute('layout-css')) {
                    children.push(createNodes(child));
                }
            }
        }
        return children;
    }
    return {
        style: parseStyle(el.getAttribute('layout-css')),
        children: getChildNodes(el),
        element: el
    };
}

The element property gives the pointer back to the SVG nodes from the layout tree, allowing us to apply the result of layout back to our SVG. However, this code does not use the 'public' JS API.

With the current public API the extract step that I want to eliminate results in a third tree, i.e.:

I have my SVG tree that I want to apply the result of css-layout to
I create a layout tree that I pass to css-layout, this has pointers back to the SVG nodes
The current public API returns a third tree with the calculated layout.

I cannot link the nodes in (3) back to (1).

I hope that makes sense!

> The design I use for react-native is that there are two trees: That's very similar to our approach with d3fc, we have the SVG 'DOM' node tree which we cannot add a `layout` property to, so we construct a node tree for the purposes of layout: ``` // creates the structure required by the layout engine function createNodes(el) { function getChildNodes() { var children = []; for (var i = 0; i < el.childNodes.length; i++) { var child = el.childNodes[i]; if (child.nodeType === 1) { if (child.getAttribute('layout-css')) { children.push(createNodes(child)); } } } return children; } return { style: parseStyle(el.getAttribute('layout-css')), children: getChildNodes(el), element: el }; } ``` The `element` property gives the pointer back to the SVG nodes from the layout tree, allowing us to apply the result of layout back to our SVG. However, this code does not use the 'public' JS API. With the current public API the `extract` step that I want to eliminate results in a third tree, i.e.: 1. I have my SVG tree that I want to apply the result of css-layout to 2. I create a layout tree that I pass to css-layout, this has pointers back to the SVG nodes 3. The current public API returns a third tree with the calculated layout. I cannot link the nodes in (3) back to (1). I hope that makes sense!

fabslab commented

2015-08-16 12:33:26 -07:00

(Migrated from github.com)

This looks like a step in the right direction and addresses a lot of the existing issues. It's interesting to see how projects like ReactCanvas have also worked around difficulties with the library - https://github.com/Flipboard/react-canvas/blob/master/lib/layoutNode.js Hopefully that can be avoided more often after these changes.

vjeux commented

2015-08-16 13:06:32 -07:00

(Migrated from github.com)

Feel free to merge it in and iterate on the api :)

ColinEberhardt commented

2015-08-17 03:50:25 -07:00

(Migrated from github.com)

Thanks for the info @fsbdev - I can see that ReactCanvas are also not using the public API for the same reasons as d3fc. Hopefully this change will make that possible.

fabslab commented

2015-08-19 18:56:10 -07:00

(Migrated from github.com)

Can you publish 0.0.3 to npm?

ColinEberhardt commented

2015-08-19 22:14:38 -07:00

(Migrated from github.com)

I've been trying to automate the npm publish via TravisCI, see https://github.com/facebook/css-layout/pull/113 - but have manually published 0.0.3 to npm just to get things moving.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#108