StockRow height, background color updates

StockRow height, background color updates

Make stock2 exactly match the original stock demo.

R=ianh@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/597f4c6403d072c7df169e6450d330ebe72756af

[hansmuller, 2015-06-15 17:11:25]

hansmuller@chromium.org changed reviewers:
+ abarth@chromium.org

[hansmuller, 2015-06-15 17:16:02]

Patchset #1 (id:1) has been deleted

[hansmuller, 2015-06-15 17:44:47]

hansmuller@chromium.org changed reviewers:
+ ianh@google.com

[hansmuller, 2015-06-15 17:49:32]

hansmuller@chromium.org changed reviewers:
+ hansmuller@chromium.org

[hansmuller, 2015-06-15 17:49:33]

PTAL. I ended up plumbing the StockList's background color through since it
seemed odd to set it in Scrollable.

[Hixie, 2015-06-15 17:54:34]

On 2015/06/15 at 17:49:33, hansmuller wrote:
> PTAL. I ended up plumbing the StockList's background color through since it
seemed odd to set it in Scrollable.

This breaks the tests. Don't forget to update the expectation files, if the
tests aren't wrong. :-)

[Hixie, 2015-06-15 17:54:39]

lgtm

https://codereview.chromium.org/1187563006/diff/60001/sky/examples/stocks2/li...
File sky/examples/stocks2/lib/stock_list.dart (right):

https://codereview.chromium.org/1187563006/diff/60001/sky/examples/stocks2/li...
sky/examples/stocks2/lib/stock_list.dart:17: }) : super(itemHeight:
StockRow.kHeight, backgroundColor: const Color(0xFFFFFFFF), key: key);
Why is the default material background colour not better?

https://codereview.chromium.org/1187563006/diff/60001/sky/examples/stocks2/li...
File sky/examples/stocks2/lib/stock_row.dart (right):

https://codereview.chromium.org/1187563006/diff/60001/sky/examples/stocks2/li...
sky/examples/stocks2/lib/stock_row.dart:20: static const double kHeight = 79.0;
We should figure out how to dynamically measure this.

https://codereview.chromium.org/1187563006/diff/60001/sky/sdk/lib/widgets/scr...
File sky/sdk/lib/widgets/scrollable.dart (right):

https://codereview.chromium.org/1187563006/diff/60001/sky/sdk/lib/widgets/scr...
sky/sdk/lib/widgets/scrollable.dart:31: void syncFields(Scrollable source) { }
You need to sync the background color.

https://codereview.chromium.org/1187563006/diff/60001/sky/sdk/lib/widgets/scr...
sky/sdk/lib/widgets/scrollable.dart:33: Color backgroundColor;
This should go immediately below the constructor.

[abarth-chromium, 2015-06-15 18:02:19]

https://codereview.chromium.org/1187563006/diff/60001/sky/examples/stocks2/li...
File sky/examples/stocks2/lib/stock_list.dart (right):

https://codereview.chromium.org/1187563006/diff/60001/sky/examples/stocks2/li...
sky/examples/stocks2/lib/stock_list.dart:17: }) : super(itemHeight:
StockRow.kHeight, backgroundColor: const Color(0xFFFFFFFF), key: key);
On 2015/06/15 at 17:54:39, Hixie wrote:
> Why is the default material background colour not better?

I suspect the stocks1 app is wrong and we should use the default material color.

[hansmuller, 2015-06-15 18:28:11]

On 2015/06/15 17:54:34, Hixie wrote:
> On 2015/06/15 at 17:49:33, hansmuller wrote:
> > PTAL. I ended up plumbing the StockList's background color through since it
> seemed odd to set it in Scrollable.
> 
> This breaks the tests. Don't forget to update the expectation files, if the
> tests aren't wrong. :-)

Sorry about that. The expectation file diff was my StockList background color
change.

[hansmuller, 2015-06-15 18:29:08]

https://codereview.chromium.org/1187563006/diff/60001/sky/examples/stocks2/li...
File sky/examples/stocks2/lib/stock_list.dart (right):

https://codereview.chromium.org/1187563006/diff/60001/sky/examples/stocks2/li...
sky/examples/stocks2/lib/stock_list.dart:17: }) : super(itemHeight:
StockRow.kHeight, backgroundColor: const Color(0xFFFFFFFF), key: key);
On 2015/06/15 18:02:18, abarth wrote:
> On 2015/06/15 at 17:54:39, Hixie wrote:
> > Why is the default material background colour not better?
> 
> I suspect the stocks1 app is wrong and we should use the default material
color.

OK. I've left the backgroundColor plumbing in place, let me know if you'd rather
I backed it out.

https://codereview.chromium.org/1187563006/diff/60001/sky/examples/stocks2/li...
File sky/examples/stocks2/lib/stock_row.dart (right):

https://codereview.chromium.org/1187563006/diff/60001/sky/examples/stocks2/li...
sky/examples/stocks2/lib/stock_row.dart:20: static const double kHeight = 79.0;
On 2015/06/15 17:54:39, Hixie wrote:
> We should figure out how to dynamically measure this.

I agree. We shouldn't be wiring down the heights of things like this. I'd like
to try to improve that in a followup CL[s].

https://codereview.chromium.org/1187563006/diff/60001/sky/sdk/lib/widgets/scr...
File sky/sdk/lib/widgets/scrollable.dart (right):

https://codereview.chromium.org/1187563006/diff/60001/sky/sdk/lib/widgets/scr...
sky/sdk/lib/widgets/scrollable.dart:31: void syncFields(Scrollable source) { }
On 2015/06/15 17:54:39, Hixie wrote:
> You need to sync the background color.

Done.

https://codereview.chromium.org/1187563006/diff/60001/sky/sdk/lib/widgets/scr...
sky/sdk/lib/widgets/scrollable.dart:33: Color backgroundColor;
On 2015/06/15 17:54:39, Hixie wrote:
> This should go immediately below the constructor.

Done.

[hansmuller, 2015-06-15 19:05:55]

Patchset #5 (id:100001) has been deleted

[Hixie, 2015-06-15 19:06:39]

lgtm

[hansmuller1, 2015-06-15 19:09:47]

Committed patchset #5 (id:120001) manually as
597f4c6403d072c7df169e6450d330ebe72756af (presubmit successful).