MD Settings: Internet: Use custom a11y labels for network toggle buttons

MD Settings: Internet: Use custom a11y labels for network toggle buttons

BUG=673944
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2707033006
Cr-Commit-Position: refs/heads/master@{#452671}
Committed: https://chromium.googlesource.com/chromium/src/+/a2f40415f68f600ff00656187b0f091ccfb5a6bb

[stevenjb, 2017-02-23 00:01:11]

Description was changed from

==========
MD Settings: Internet: Use custom a11y labels for network toggle buttons

BUG=673944
==========

to

==========
MD Settings: Internet: Use custom a11y labels for network toggle buttons

BUG=673944
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2017-02-23 00:03:40]

stevenjb@chromium.org changed reviewers:
+ hcarmona@chromium.org

[stevenjb, 2017-02-23 00:03:41]

hcarmona@ - Apologies, when I reviewed your {itemName} change that seemed like
the right string to me at the time, but in making the internet section more
consistent with the un-inlined Bluetooth section I realized that we should
follow the same pattern we use there and add custom strings for these toggles.

[hcarmona, 2017-02-23 00:52:22]

On 2017/02/23 00:03:41, stevenjb wrote:
> hcarmona@ - Apologies, when I reviewed your {itemName} change that seemed like
> the right string to me at the time, but in making the internet section more
> consistent with the un-inlined Bluetooth section I realized that we should
> follow the same pattern we use there and add custom strings for these toggles.

No worries, new strings look good.

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_summary_item.js
(left):

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_summary_item.js:35:
itemName: String,
itemName can also be removed from cr-network-list-item.

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_summary_item.js
(right):

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_summary_item.js:16:
behaviors: [Polymer.IronA11yKeysBehavior, I18nBehavior],
Another possibility is to use $i18nPolymer{...} in the HTML to pass in the
string values similar to this:
https://codereview.chromium.org/2693373002/diff/1/chrome/browser/resources/se...

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_summary_item.js:250:
return '';
Does it make sense to return "Unknown device enable" or to add an
assertNotReached() here?

[stevenjb, 2017-02-23 20:51:33]

ptal

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_summary_item.js
(left):

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_summary_item.js:35:
itemName: String,
On 2017/02/23 00:52:21, hcarmona wrote:
> itemName can also be removed from cr-network-list-item.

I saw that but decided that leaving itemName as a computed property seems fine.

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_summary_item.js
(right):

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_summary_item.js:16:
behaviors: [Polymer.IronA11yKeysBehavior, I18nBehavior],
On 2017/02/23 00:52:21, hcarmona wrote:
> Another possibility is to use $i18nPolymer{...} in the HTML to pass in the
> string values similar to this:
>
https://codereview.chromium.org/2693373002/diff/1/chrome/browser/resources/se...

There are currently 3 and eventually 4 strings (when we add a 'Tether' type)
that would need to be passed in, which seems like it would be pretty unwieldy in
the HTML for a relatively minor optimization.

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_summary_item.js:250:
return '';
On 2017/02/23 00:52:21, hcarmona wrote:
> Does it make sense to return "Unknown device enable" or to add an
> assertNotReached() here?

I'd rather not added a string we don't plan to use, but I can early exit when we
expect the control to be hidden and add an assert.

[hcarmona, 2017-02-23 21:51:06]

LGTM

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_summary_item.js
(right):

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_summary_item.js:16:
behaviors: [Polymer.IronA11yKeysBehavior, I18nBehavior],
On 2017/02/23 20:51:33, stevenjb wrote:
> On 2017/02/23 00:52:21, hcarmona wrote:
> > Another possibility is to use $i18nPolymer{...} in the HTML to pass in the
> > string values similar to this:
> >
>
https://codereview.chromium.org/2693373002/diff/1/chrome/browser/resources/se...
> 
> There are currently 3 and eventually 4 strings (when we add a 'Tether' type)
> that would need to be passed in, which seems like it would be pretty unwieldy
in
> the HTML for a relatively minor optimization.
> 

Sounds good, no objections

https://codereview.chromium.org/2707033006/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_summary_item.js:250:
return '';
On 2017/02/23 20:51:33, stevenjb wrote:
> On 2017/02/23 00:52:21, hcarmona wrote:
> > Does it make sense to return "Unknown device enable" or to add an
> > assertNotReached() here?
> 
> I'd rather not added a string we don't plan to use, but I can early exit when
we
> expect the control to be hidden and add an assert.

Awesome, thanks :-)

[stevenjb, 2017-02-23 21:59:35]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2017-02-23 22:00:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 23:22:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487887175382920,
"parent_rev": "8ef4aa62f40aa8ef1ab4d6841102f8bf1d931bf2", "commit_rev":
"a2f40415f68f600ff00656187b0f091ccfb5a6bb"}

[commit-bot: I haz the power, 2017-02-23 23:23:16]

Description was changed from

==========
MD Settings: Internet: Use custom a11y labels for network toggle buttons

BUG=673944
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Internet: Use custom a11y labels for network toggle buttons

BUG=673944
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2707033006
Cr-Commit-Position: refs/heads/master@{#452671}
Committed:
https://chromium.googlesource.com/chromium/src/+/a2f40415f68f600ff00656187b0f...
==========

[commit-bot: I haz the power, 2017-02-23 23:23:17]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a2f40415f68f600ff00656187b0f...