Card "swipe-away" dismiss version 3: Uses BlockViewport

Card "swipe-away" dismiss version 3: Uses BlockViewport

Scrollable version of the existing demo.

Includes Ian's BlockViewport fixes.

Still TODO: track the layout and update the ScrollBehavior's
contentsHeight as needed. Stop when we've reached the need.

R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/f3f5045179ccd0ebf00e4d1f8deece265d2c2e9a

[hansmuller, 2015-07-09 17:58:26]

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

[hansmuller, 2015-07-09 17:59:48]

Patchset #1 (id:1) has been deleted

[hansmuller, 2015-07-09 23:58:25]

Patchset #1 (id:20001) has been deleted

[hansmuller, 2015-07-09 23:58:33]

Patchset #1 (id:40001) has been deleted

[eseidel, 2015-07-10 00:06:00]

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

[eseidel, 2015-07-10 00:06:00]

https://codereview.chromium.org/1227963003/diff/60001/sky/sdk/lib/widgets/blo...
File sky/sdk/lib/widgets/block_viewport.dart (right):

https://codereview.chromium.org/1227963003/diff/60001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:103: bool _dirty = true;
Did you mean to change this?

[hansmuller, 2015-07-10 00:07:58]

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

[hansmuller, 2015-07-10 00:07:58]

https://codereview.chromium.org/1227963003/diff/60001/sky/sdk/lib/widgets/blo...
File sky/sdk/lib/widgets/block_viewport.dart (right):

https://codereview.chromium.org/1227963003/diff/60001/sky/sdk/lib/widgets/blo...
sky/sdk/lib/widgets/block_viewport.dart:103: bool _dirty = true;
On 2015/07/10 00:06:00, eseidel wrote:
> Did you mean to change this?

Yes, that's the way it's supposed to be.

[Hixie, 2015-07-10 00:08:14]

ianh@google.com changed reviewers:
- eseidel@chromium.org, hansmuller@chromium.org

[Hixie, 2015-07-10 00:08:15]

https://codereview.chromium.org/1227963003/diff/60001/sky/sdk/example/widgets...
File sky/sdk/example/widgets/card_collection.dart (right):

https://codereview.chromium.org/1227963003/diff/60001/sky/sdk/example/widgets...
sky/sdk/example/widgets/card_collection.dart:29: class CardCollection extends
Scrollable {
I think this should just be a generic ValiableHeightScrollable class that takes
a builder and a token.

https://codereview.chromium.org/1227963003/diff/60001/sky/sdk/example/widgets...
sky/sdk/example/widgets/card_collection.dart:33: int this.cardCount
take a token, not a count. If an item in the list is replaced with a new one, or
if an item in the list changes height, then the token should change, even though
the count doesn't.

https://codereview.chromium.org/1227963003/diff/60001/sky/sdk/example/widgets...
sky/sdk/example/widgets/card_collection.dart:65: );
the padding and decoration should be done by the parent.

Also, for consistency with other classes, always have the child hierarchy
indented with each child on a new line. That way we can trivially see what the
structure is.

[hansmuller, 2015-07-10 00:18:50]

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

[hansmuller, 2015-07-10 00:18:50]

https://codereview.chromium.org/1227963003/diff/60001/sky/sdk/example/widgets...
File sky/sdk/example/widgets/card_collection.dart (right):

https://codereview.chromium.org/1227963003/diff/60001/sky/sdk/example/widgets...
sky/sdk/example/widgets/card_collection.dart:29: class CardCollection extends
Scrollable {
On 2015/07/10 00:08:15, Hixie wrote:
> I think this should just be a generic ValiableHeightScrollable class that
takes
> a builder and a token.

Done.

https://codereview.chromium.org/1227963003/diff/60001/sky/sdk/example/widgets...
sky/sdk/example/widgets/card_collection.dart:33: int this.cardCount
On 2015/07/10 00:08:15, Hixie wrote:
> take a token, not a count. If an item in the list is replaced with a new one,
or
> if an item in the list changes height, then the token should change, even
though
> the count doesn't.

Done.

https://codereview.chromium.org/1227963003/diff/60001/sky/sdk/example/widgets...
sky/sdk/example/widgets/card_collection.dart:65: );
On 2015/07/10 00:08:15, Hixie wrote:
> the padding and decoration should be done by the parent.
> 
> Also, for consistency with other classes, always have the child hierarchy
> indented with each child on a new line. That way we can trivially see what the
> structure is.

Done.

[abarth-chromium, 2015-07-10 02:37:27]

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

[abarth-chromium, 2015-07-10 02:37:28]

lgtm

https://codereview.chromium.org/1227963003/diff/80001/sky/sdk/example/widgets...
File sky/sdk/example/widgets/card_collection.dart (right):

https://codereview.chromium.org/1227963003/diff/80001/sky/sdk/example/widgets...
sky/sdk/example/widgets/card_collection.dart:29: class VariableHeightScrollable
extends Scrollable {
This class came out great.  Nice and clean.

https://codereview.chromium.org/1227963003/diff/80001/sky/sdk/example/widgets...
sky/sdk/example/widgets/card_collection.dart:51: scrollBehavior.contentsHeight =
5000.0;
Where does this number come from?  Maybe BlockViewport should have a callback
with some sizing information?

https://codereview.chromium.org/1227963003/diff/80001/sky/sdk/example/widgets...
sky/sdk/example/widgets/card_collection.dart:61: return new SizeObserver(child:
viewport, callback: _handleSizeChanged);
The style we've used in other framework widgets is to just make a big
constructor cascade:

return new SizeObserver(
  callback: _handleSizeChanged,
  child: new BlockViewport(
    builder: builder,
    startOffset: scrollOffset,
    token: token
  )
);

I'm not sure it's the best style, but if the idea is to lift this widget out of
this example and into the framework we probably should be consistent.

[hansmuller, 2015-07-10 15:45:58]

https://codereview.chromium.org/1227963003/diff/80001/sky/sdk/example/widgets...
File sky/sdk/example/widgets/card_collection.dart (right):

https://codereview.chromium.org/1227963003/diff/80001/sky/sdk/example/widgets...
sky/sdk/example/widgets/card_collection.dart:51: scrollBehavior.contentsHeight =
5000.0;
On 2015/07/10 02:37:27, abarth-chromium wrote:
> Where does this number come from?  Maybe BlockViewport should have a callback
> with some sizing information?

Yes. For this case maybe BlockViewport just needs to expose some state (like the
item offsets array and the startIndex). To enable something like
myVariableHeightScrollable.ensureItemIsVisible(index) the implementation might
have to construct BlockViewport more than once. I will take a crack at this when
I move VariableHeightScrollable out of the example.

https://codereview.chromium.org/1227963003/diff/80001/sky/sdk/example/widgets...
sky/sdk/example/widgets/card_collection.dart:61: return new SizeObserver(child:
viewport, callback: _handleSizeChanged);
On 2015/07/10 02:37:27, abarth-chromium wrote:
> The style we've used in other framework widgets is to just make a big
> constructor cascade:
> 
> return new SizeObserver(
>   callback: _handleSizeChanged,
>   child: new BlockViewport(
>     builder: builder,
>     startOffset: scrollOffset,
>     token: token
>   )
> );
> 
> I'm not sure it's the best style, but if the idea is to lift this widget out
of
> this example and into the framework we probably should be consistent.

Sure, I'll conform. I've tried to limit the depth of these constructions because
I don't find super-long cascades of close paren lines terribly readable.

[hansmuller1, 2015-07-10 15:46:26]

Committed patchset #3 (id:100001) manually as
f3f5045179ccd0ebf00e4d1f8deece265d2c2e9a (presubmit successful).