Example in readme does not run. #24

New Issue

2015-01-29T22:37:01-08:00

jimmyhmiller commented

2015-01-29 22:37:01 -08:00

(Migrated from github.com)

Running the example in the readme will result in "node.layout is undefined" error.

computeLayout(
  {style: {padding: 50}, children: [
    {style: {padding: 10, alignSelf: 'stretch'}}
  ]}
);

This doesn't occur in the tests because of the preprocessing the node goes through in the testing suite. Particularly in the computeCSSLayout function.

I'm not sure if this is the expected behavior for the library or if the preprocessing should be moved internally, but either way the readme example should change to reflect this.

Running the example in the readme will result in "node.layout is undefined" error. ``` javascript computeLayout( {style: {padding: 50}, children: [ {style: {padding: 10, alignSelf: 'stretch'}} ]} ); ``` This doesn't occur in the tests because of the preprocessing the node goes through in the testing suite. Particularly in the [computeCSSLayout](https://github.com/facebook/css-layout/blob/28243156e48cc6e3cb8adb809e03406c5970eb41/src/Layout-test-utils.js#L131) function. I'm not sure if this is the expected behavior for the library or if the preprocessing should be moved internally, but either way the readme example should change to reflect this.

dashed commented

2015-01-29 23:15:21 -08:00

(Migrated from github.com)

This is how I run them 😄

var computeLayout = testLayout = require('./Layout.js');

// Utils -- ripped from tests

  if (typeof computeLayout === 'function') {
    var realComputeLayout = computeLayout;
  }

  function roundLayout(layout) {
    // Chrome rounds all the numbers with a precision of 1/64
    // Reproduce the same behavior
    function round(number) {
      var floored = Math.floor(number);
      var decimal = number - floored;
      if (decimal === 0) {
        return number;
      }
      var minDifference = Infinity;
      var minDecimal = Infinity;
      for (var i = 1; i < 64; ++i) {
        var roundedDecimal = i / 64;
        var difference = Math.abs(roundedDecimal - decimal);
        if (difference < minDifference) {
          minDifference = difference;
          minDecimal = roundedDecimal;
        }
      }
      return floored + minDecimal;
    }

    function rec(layout) {
      layout.top = round(layout.top);
      layout.left = round(layout.left);
      layout.width = round(layout.width);
      layout.height = round(layout.height);
      if (layout.children) {
        for (var i = 0; i < layout.children.length; ++i) {
          rec(layout.children[i]);
        }
      }
    }

    rec(layout);
    return layout;
  }

  function computeCSSLayout(rootNode) {
    function fillNodes(node) {
      node.layout = {
        width: undefined,
        height: undefined,
        top: 0,
        left: 0
      };
      if (!node.style) {
        node.style = {};
      }

      if (!node.children || node.style.measure) {
        node.children = [];
      }
      node.children.forEach(fillNodes);
    }

    function extractNodes(node) {
      var layout = node.layout;
      delete node.layout;
      if (node.children.length > 0) {
        layout.children = node.children.map(extractNodes);
      } else {
        delete node.children;
      }
      return layout;
    }

    fillNodes(rootNode);
    realComputeLayout(rootNode);
    return roundLayout(extractNodes(rootNode));
  }

///////

var node = {
    style: {padding: 50},
    children: [{style: {padding: 10, alignSelf: 'stretch'}}]
};

var result = computeCSSLayout(node);

This is how I run them :smile: ``` js var computeLayout = testLayout = require('./Layout.js'); // Utils -- ripped from tests if (typeof computeLayout === 'function') { var realComputeLayout = computeLayout; } function roundLayout(layout) { // Chrome rounds all the numbers with a precision of 1/64 // Reproduce the same behavior function round(number) { var floored = Math.floor(number); var decimal = number - floored; if (decimal === 0) { return number; } var minDifference = Infinity; var minDecimal = Infinity; for (var i = 1; i < 64; ++i) { var roundedDecimal = i / 64; var difference = Math.abs(roundedDecimal - decimal); if (difference < minDifference) { minDifference = difference; minDecimal = roundedDecimal; } } return floored + minDecimal; } function rec(layout) { layout.top = round(layout.top); layout.left = round(layout.left); layout.width = round(layout.width); layout.height = round(layout.height); if (layout.children) { for (var i = 0; i < layout.children.length; ++i) { rec(layout.children[i]); } } } rec(layout); return layout; } function computeCSSLayout(rootNode) { function fillNodes(node) { node.layout = { width: undefined, height: undefined, top: 0, left: 0 }; if (!node.style) { node.style = {}; } if (!node.children || node.style.measure) { node.children = []; } node.children.forEach(fillNodes); } function extractNodes(node) { var layout = node.layout; delete node.layout; if (node.children.length > 0) { layout.children = node.children.map(extractNodes); } else { delete node.children; } return layout; } fillNodes(rootNode); realComputeLayout(rootNode); return roundLayout(extractNodes(rootNode)); } /////// var node = { style: {padding: 50}, children: [{style: {padding: 10, alignSelf: 'stretch'}}] }; var result = computeCSSLayout(node); ```

vjeux commented

2015-01-30 11:35:08 -08:00

(Migrated from github.com)

Woops. We're not actually using the JS version anywhere so the code isn't in a very good shape. Wanna submit a pull request to make it cleaner?

ColinEberhardt commented

2015-02-02 14:06:48 -08:00

(Migrated from github.com)

I was just about to raise exactly this issue! The computeLayout function relies on the nodes having a layout property. This is ensures within the tests as a result of the fillNodes function.

Personally I think the API would be better if computeLayout was a bit more flexible - adding the layout property if it is not already present.

I was just about to raise exactly this issue! The `computeLayout` function relies on the nodes having a `layout` property. This is ensures within the tests as a result of the `fillNodes` function. Personally I think the API would be better if `computeLayout` was a bit more flexible - adding the `layout` property if it is not already present.

vjeux commented

2015-02-02 14:08:01 -08:00

(Migrated from github.com)

@ColinEberhardt: happy to take pull requests to improve the JS API. It's indeed not great in the current form

ColinEberhardt commented

2015-02-02 14:12:23 -08:00

(Migrated from github.com)

@vjeux - I'll see what I can do :-)

By the way, I'm using code from this project to apply flexbox layout to SVG - it's great for creating chart layouts. You can see this in practice here:

http://www.scottlogic.com/blog/2015/02/02/svg-layout-flexbox.html

Thanks for making this open source.

@vjeux - I'll see what I can do :-) By the way, I'm using code from this project to apply flexbox layout to SVG - it's great for creating chart layouts. You can see this in practice here: http://www.scottlogic.com/blog/2015/02/02/svg-layout-flexbox.html Thanks for making this open source.

dashed commented

2015-02-02 14:15:53 -08:00

(Migrated from github.com)

There seems to be more missing pieces, like text sizing. At the moment these are hardcoded in tests, and rely on some iframe hack to get initial values.

vjeux commented

2015-02-02 14:18:22 -08:00

(Migrated from github.com)

@Dashed: in the test files there's a function that is able to measure text. Unfortunately, the browser support for that is pretty bad. You can either use canvas, but then you don't have line wrapping, or create a dom node yourself but it's pretty slow.

vjeux commented

2015-02-02 14:22:25 -08:00

(Migrated from github.com)

@ColinEberhardt this is really exciting, thanks for letting me know :) I had no idea that anyone else would be using it to do interesting things

1genius commented

2015-02-04 04:49:29 -08:00

(Migrated from github.com)

I would like to try and fix the bug?

amirouche commented

2015-02-08 13:28:00 -08:00

(Migrated from github.com)

The layout will fail if a child miss "style" attribute cf. https://gist.github.com/amirouche/c756a6bb4de2aa95e958#file-app-js-L18

fczuardi commented

2015-02-13 06:51:15 -08:00

(Migrated from github.com)

the workaround for now is to use @jimmyhmiller's fork that includes the fillNodes function in the library and exposes it, it would be better to also have the extractNodes from @Dashed in there as well so that the result is consistent with what the README says the output will be…

So, here is what I am doing as a workaround for using css-layout in a javascript project:

$ npm uninstall css-layout
$ npm install -D jimmyhmiller/css-layout

Then, in the javascript:

var CSSLayout = require('css-layout'),
    computeLayout = CSSLayout.computeLayout,
    fillNodes = CSSLayout.fillNodes;

var rootNode = {
    style: {padding: 50},
    children: [
        {style: {padding: 10, alignSelf: 'stretch'}}
    ]
};

fillNodes(rootNode);
computeLayout(rootNode);

console.log(rootNode);

the workaround for now is to use @jimmyhmiller's fork that includes the fillNodes function in the library and exposes it, it would be better to also have the extractNodes from @Dashed in there as well so that the result is consistent with what the README says the output will be… So, here is what I am doing as a workaround for using css-layout in a javascript project: ``` bash $ npm uninstall css-layout $ npm install -D jimmyhmiller/css-layout ``` Then, in the javascript: ``` javascript var CSSLayout = require('css-layout'), computeLayout = CSSLayout.computeLayout, fillNodes = CSSLayout.fillNodes; var rootNode = { style: {padding: 50}, children: [ {style: {padding: 10, alignSelf: 'stretch'}} ] }; fillNodes(rootNode); computeLayout(rootNode); console.log(rootNode); ```

jimmyhmiller commented

2015-02-13 08:00:32 -08:00

(Migrated from github.com)

My goal is to have my repo in a state where I can make a pull request this weekend. I'm hoping to have some time Sunday to work on it.

pandeiro commented

2015-02-14 09:58:34 -08:00

(Migrated from github.com)

+1 @jimmyhmiller @Dashed @ColinEberhardt

Glad I found this issue -- we'd like to use css-layout from clojurescript so a fix for easier use from JS is most welcome!

+1 @jimmyhmiller @Dashed @ColinEberhardt Glad I found this issue -- we'd like to use css-layout from [clojurescript](https://github.com/cljsjs/packages/tree/master/css-layout) so a fix for easier use from JS is most welcome!

jimmyhmiller commented

2015-02-16 05:11:56 -08:00

(Migrated from github.com)

Since my PR was merged this example now runs. @pandeiro let me know if there is anything you find you. I'm excited to see what everyone is building with this, got some ideas myself.

dashed commented

2015-02-16 05:26:18 -08:00

(Migrated from github.com)

@jimmyhmiller Thanks for the refactor 👍

@jimmyhmiller Thanks for the refactor :+1:

amirouche commented

2015-05-30 07:10:40 -07:00

(Migrated from github.com)

was the PR merged?

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#24