[package:mdv] Support binding to HTMLSelectElement.selectedIndex

[package:mdv] Support binding to HTMLSelectElement.selectedIndex -- https://github.com/Polymer/mdv/commit/53a5d78b329b9a52ad9426bbd57b9322f42f1bd2

R=sigmund@google.com

Committed: https://code.google.com/p/dart/source/detail?r=24667

[Jennifer Messerly, 2013-06-27 23:59:48]

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/bindings.dart
File pkg/mdv/lib/src/bindings.dart (left):

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/bindings.d...
pkg/mdv/lib/src/bindings.dart:68: class _ValueBinding extends _InputBinding {
these just moved to input_element

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/input_elem...
File pkg/mdv/lib/src/input_element.dart (right):

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/input_elem...
pkg/mdv/lib/src/input_element.dart:62: class _ValueBinding extends _InputBinding
{
these moved from
https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/bindings.dart

[Jennifer Messerly, 2013-06-28 18:56:47]

+Siggi since Justin is out sick

[Siggi Cherem (dart-lang), 2013-06-28 20:36:13]

nice

minor nits and mainly questions =)

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/select_ele...
File pkg/mdv/lib/src/select_element.dart (right):

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/select_ele...
pkg/mdv/lib/src/select_element.dart:11: SelectElement get node => super.node;
is this needed for functionality or just for specializing the type?

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/select_ele...
pkg/mdv/lib/src/select_element.dart:17: case 'selectedindex':
seems strange to have a switch statement for just 1 value, use if-else?  (same
below)

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/select_ele...
pkg/mdv/lib/src/select_element.dart:55: var newValue = _toInt(value);
not for now, but I wonder how many bindings typically convert from one type to
another (I'm thinking value-as-number, for instance) and whether it is useful to
have something like a int/num binding converter (either as a base class or a
wrapper)

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/select_ele...
pkg/mdv/lib/src/select_element.dart:63: // iterating <optgroup> and once for
<option>.
is this to address this bug: https://github.com/dart-lang/web-ui/issues/519?

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/select_ele...
pkg/mdv/lib/src/select_element.dart:86: if (value is String) return
int.parse(value, onError: (_) {});
maybe make it more explicit that the onerror default is null by doing:
 "onError: (_) => null"

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/test/element_bindi...
File pkg/mdv/test/element_bindings_test.dart (right):

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/test/element_bindi...
pkg/mdv/test/element_bindings_test.dart:391: });
if the delay has the effect of fixing #519, could we add that as a regression
test here too =)?

[Jennifer Messerly, 2013-07-01 22:07:25]

Thanks! PTAL

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/select_ele...
File pkg/mdv/lib/src/select_element.dart (right):

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/select_ele...
pkg/mdv/lib/src/select_element.dart:11: SelectElement get node => super.node;
On 2013/06/28 20:36:13, Siggi Cherem (dart-lang) wrote:
> is this needed for functionality or just for specializing the type?

just specializing the type. better type checks, mmmm :)

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/select_ele...
pkg/mdv/lib/src/select_element.dart:17: case 'selectedindex':
On 2013/06/28 20:36:13, Siggi Cherem (dart-lang) wrote:
> seems strange to have a switch statement for just 1 value, use if-else?  (same
> below)

good point. fixed!

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/select_ele...
pkg/mdv/lib/src/select_element.dart:55: var newValue = _toInt(value);
On 2013/06/28 20:36:13, Siggi Cherem (dart-lang) wrote:
> not for now, but I wonder how many bindings typically convert from one type to
> another (I'm thinking value-as-number, for instance) and whether it is useful
to
> have something like a int/num binding converter (either as a base class or a
> wrapper)

it's a good point. I added a TODO to highlight this.

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/select_ele...
pkg/mdv/lib/src/select_element.dart:63: // iterating <optgroup> and once for
<option>.
On 2013/06/28 20:36:13, Siggi Cherem (dart-lang) wrote:
> is this to address this bug: https://github.com/dart-lang/web-ui/issues/519?

i don't think it is related. it's to make sure <template repeat> expands the
options before we try to select it. otherwise setting selectedIndex has no
effect

https://codereview.chromium.org/18117012/diff/2001/pkg/mdv/lib/src/select_ele...
pkg/mdv/lib/src/select_element.dart:86: if (value is String) return
int.parse(value, onError: (_) {});
On 2013/06/28 20:36:13, Siggi Cherem (dart-lang) wrote:
> maybe make it more explicit that the onerror default is null by doing:
>  "onError: (_) => null"

Done.

[Siggi Cherem (dart-lang), 2013-07-01 22:23:09]

lgtm!

[Jennifer Messerly, 2013-07-01 22:38:29]

Committed patchset #4 manually as r24667 (presubmit successful).