Rework javascript api to match README #50

jimmyhmiller · 2015-02-16T02:02:28Z

This is a fix for #24

It doesn't require any changes to the transpiled code, but exposes a nice api for javascript. I did have to create a new file as the main for package.json. I'd love to work on simplifying the structure of the javascript in the future. Perhaps moving the layout algorithm into its own file. But those sorts of changes can be done later.

Let me know if you have any changes you'd like me to make.

vjeux · 2015-02-16T02:07:56Z

So good, thanks for doing that. Please keep the pull requests coming for improving the js api :)

Rework javascript api to match README

Summary: This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:facebook/yoga#50 style C fill:facebook/yoga#50 style D fill:facebook/yoga#50 style H fill:facebook/yoga#50 style I fill:facebook/yoga#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on facebook/yoga#1725 X-link: facebook/yoga#1726 Differential Revision: D64404340 Pulled By: NickGerleman

Summary: This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:facebook/yoga#50 style C fill:facebook/yoga#50 style D fill:facebook/yoga#50 style H fill:facebook/yoga#50 style I fill:facebook/yoga#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on facebook/yoga#1725 X-link: facebook/yoga#1726 Test Plan: Added tests for `display: contents` based on existing tests for `display: none` and ensured that all the tests were passing. Differential Revision: D64404340 Pulled By: NickGerleman

Summary: This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:facebook/yoga#50 style C fill:facebook/yoga#50 style D fill:facebook/yoga#50 style H fill:facebook/yoga#50 style I fill:facebook/yoga#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on facebook/yoga#1725 Changelog: [Internal] X-link: facebook/yoga#1726 Test Plan: Added tests for `display: contents` based on existing tests for `display: none` and ensured that all the tests were passing. Differential Revision: D64404340 Pulled By: NickGerleman

Summary: X-link: facebook/react-native#47035 This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:#50 style C fill:#50 style D fill:#50 style H fill:#50 style I fill:#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on #1725 Changelog: [Internal] Pull Request resolved: #1726 Test Plan: Added tests for `display: contents` based on existing tests for `display: none` and ensured that all the tests were passing. Reviewed By: joevilches Differential Revision: D64404340 Pulled By: NickGerleman fbshipit-source-id: f6f6e9a6fad82873f18c8a0ead58aad897df5d09

Summary: Pull Request resolved: #47035 This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:facebook/yoga#50 style C fill:facebook/yoga#50 style D fill:facebook/yoga#50 style H fill:facebook/yoga#50 style I fill:facebook/yoga#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on facebook/yoga#1725 Changelog: [Internal] X-link: facebook/yoga#1726 Test Plan: Added tests for `display: contents` based on existing tests for `display: none` and ensured that all the tests were passing. Reviewed By: joevilches Differential Revision: D64404340 Pulled By: NickGerleman fbshipit-source-id: f6f6e9a6fad82873f18c8a0ead58aad897df5d09

Summary: X-link: facebook/react-native#47035 This PR adds support for `display: contents` style by effectively skipping nodes with `display: contents` set during layout. This required changes in the logic related to children traversal - before this PR a node would be always laid out in the context of its direct parent. After this PR that assumption is no longer true - `display: contents` allows nodes to be skipped, i.e.: ```html <div id="node1"> <div id="node2" style="display: contents;"> <div id="node3" /> </div> </div> ``` `node3` will be laid out as if it were a child of `node1`. Because of this, iterating over direct children of a node is no longer correct to achieve the correct layout. This PR introduces `LayoutableChildren::Iterator` which can traverse the subtree of a given node in a way that nodes with `display: contents` are replaced with their concrete children. A tree like this: ```mermaid flowchart TD A((A)) B((B)) C((C)) D((D)) E((E)) F((F)) G((G)) H((H)) I((I)) J((J)) A --> B A --> C B --> D B --> E C --> F D --> G F --> H G --> I H --> J style B fill:facebook/yoga#50 style C fill:facebook/yoga#50 style D fill:facebook/yoga#50 style H fill:facebook/yoga#50 style I fill:facebook/yoga#50 ``` would be laid out as if the green nodes (ones with `display: contents`) did not exist. It also changes the logic where children were accessed by index to use the iterator instead as random access would be non-trivial to implement and it's not really necessary - the iteration was always sequential and indices were only used as boundaries. There's one place where knowledge of layoutable children is required to calculate the gap. An optimization for this is for a node to keep a counter of how many `display: contents` nodes are its children. If there are none, a short path of just returning the size of the children vector can be taken, otherwise it needs to iterate over layoutable children and count them, since the structure may be complex. One more major change this PR introduces is `cleanupContentsNodesRecursively`. Since nodes with `display: contents` would be entirely skipped during the layout pass, they would keep previous metrics, would be kept as dirty, and, in the case of nested `contents` nodes, would not be cloned, breaking `doesOwn` relation. All of this is handled in the new method which clones `contents` nodes recursively, sets empty layout, and marks them as clean and having a new layout so that it can be used on the React Native side. Relies on facebook/yoga#1725 Changelog: [Internal] X-link: facebook/yoga#1726 Reviewed By: joevilches Differential Revision: D64404340 Pulled By: NickGerleman fbshipit-source-id: f6f6e9a6fad82873f18c8a0ead58aad897df5d09

Rework javascript api to match README

8fe8c15

vjeux added a commit that referenced this pull request Feb 16, 2015

Merge pull request #50 from jimmyhmiller/master

b99ce37

Rework javascript api to match README

vjeux merged commit b99ce37 into facebook:master Feb 16, 2015

ColinEberhardt mentioned this pull request Aug 13, 2015

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework javascript api to match README #50

Rework javascript api to match README #50

jimmyhmiller commented Feb 16, 2015

vjeux commented Feb 16, 2015

Rework javascript api to match README #50

Rework javascript api to match README #50

Conversation

jimmyhmiller commented Feb 16, 2015

vjeux commented Feb 16, 2015