palantir / plottable

:bar_chart: A library of modular chart components built on D3
http://plottablejs.org/
MIT License
2.96k stars 224 forks source link

[Table] _iterateLayout behaves weirdly #2192

Open aicioara opened 9 years ago

aicioara commented 9 years ago

Code:

var c1 = new Mocks.FixedSizeComponent(490, 50);
var c2 = new Plottable.Component();
var c3 = new Plottable.Component();
var c4 = new Plottable.Component();
var table = new Plottable.Components.Table([
  [c1, c2],
  [c3, c4]
]);
var result = (<any> table)._iterateLayout(500, 500);

assert.deepEqual(result.colProportionalSpace, [5, 5], "colProportionalSpace:");
assert.deepEqual(result.rowProportionalSpace, [255, 255], "rowProportionalSpace:");
assert.deepEqual(result.guaranteedWidths, [490, 0], "guaranteedWidths:");
assert.deepEqual(result.guaranteedHeights, [50, 0], "guaranteedHeights:");
assert.deepEqual(result.wantsWidth, false, "wantsWidth:");
assert.deepEqual(result.wantsHeight, false, "wantsHeight:");

Comment: I have an odd feeling about this. In particular, the first 2 arguments [5, 5], [255, 255] represent colProportionalSpace and rowProportionalSpace respectively. I do not think this is true.

The way this should be read is: We have 2 components, they share 10 units of column space and 250 units of row space, which are split equally between them. This would be (somewhat) true if we had no fixed component. The fixed component makes the semantics of this very weird

teamdandelion commented 9 years ago

@aicioara I feel I can probably be of assistance here since I wrote this part of the layout engine, and it is indeed pretty weird. As with many things that are weird, it started with some sensible assumptions and an elegant model, and developed incremental weirdness as necessary to handle more and more complicated cases.

I'm not sure where the [255, 255] figures come from since they are inconsistent with the test case you're describing (255 + 255 > 500 so it should never get returned as row proportional space allocations when given 500 px space) and I can't find any reference to 255, 255 in source. Just looking at the git history for this test it looks like a confusing situation, I vaguely remember writing this test and it looks like last May I skipped it for some reason. I don't remember what the story is.

But I'm happy to help if I can. I am in Palo Alto pretty often so I could swing by and go over and try to demystify the table layout code. You can email me at danmane@gmail.com or get my # from Justin or DBT, I don't check Github issues super frequently.

aicioara commented 9 years ago

When I looked into it, the numbers made sense (I mean I could figure out what was going on), but that was 3 weeks ago. When I go back to it, I'l reopen this discussion.

Also please note that the code for the layout engin has been changed a lot (to my understanding) in the past couple of weeks (due to the major refactor).

If there is anything, I'll let you know. Thanks a lot!

teamdandelion commented 9 years ago

OK :)