MD Settings: Allow controlled-button to be used with icons.

MD Settings: Allow controlled-button to be used with icons.

The goal is to "replace CHANGE button with outlink icon".

-- Edit:
Rediscussed with designer, concluded on not moving forward with this change.

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

[scottchen, 2017-02-02 01:23:28]

Description was changed from

==========
MD Settings: Allow controlled-button to be used with icons.

The goal is to "replace CHANGE button with outlink icon".

BUG=684152
==========

to

==========
MD Settings: Allow controlled-button to be used with icons.

The goal is to "replace CHANGE button with outlink icon".

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

[scottchen, 2017-02-02 01:24:53]

scottchen@chromium.org changed reviewers:
+ dschuyler@chromium.org, tommycli@chromium.org

[scottchen, 2017-02-02 01:24:54]

See before after: http://imgur.com/a/XVPDv

[scottchen, 2017-02-02 01:25:19]

The CQ bit was checked by scottchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 01:25:55]

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

[tommycli, 2017-02-02 22:20:54]

lgtm sans:

https://codereview.chromium.org/2669073002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/controlled_button.js (right):

https://codereview.chromium.org/2669073002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_button.js:21: iconClass: {
Please add some commentary that if iconClass is defined, the light dom content
will be ignored.

[scottchen, 2017-02-02 22:46:42]

https://codereview.chromium.org/2669073002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/controlled_button.js (right):

https://codereview.chromium.org/2669073002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_button.js:21: iconClass: {
On 2017/02/02 22:20:54, tommycli wrote:
> Please add some commentary that if iconClass is defined, the light dom content
> will be ignored.

Done.

[scottchen, 2017-02-02 22:46:47]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-02-02 22:46:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2669073002/#ps40001
(title: "add comment about iconClass")

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

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-03 00:48:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 00:48:13]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[scottchen, 2017-02-03 01:33:36]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-02-03 01:34:34]

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

[dschuyler, 2017-02-03 01:39:30]

https://codereview.chromium.org/2669073002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/controlled_button.html (right):

https://codereview.chromium.org/2669073002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_button.html:44: actionable
class$="{{iconClass}}"></button>
Can this use one-way binding instead of two-way. e.g. change {{iconClass}} to
[[iconClass]]

https://codereview.chromium.org/2669073002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/downloads_page/downloads_page.html
(right):

https://codereview.chromium.org/2669073002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:10: <div
class="settings-box first two-line">
When using an icon-external, the entire settings-box should be clickable. e.g.
by adding an on-tap="" here.

[scottchen, 2017-02-03 01:42:06]

The CQ bit was unchecked by scottchen@chromium.org

[scottchen, 2017-02-03 22:15:56]

scottchen@chromium.org changed reviewers:
+ dbeam@chromium.org

[scottchen, 2017-02-03 22:15:57]

+ dbeam@ Wondering what your thoughts are on:
   - the code change
   - the UX change 
screenshot: (http://imgur.com/a/XVPDv) 
design discussion:
(https://docs.google.com/spreadsheets/d/17uvG_CTZ7N16B7hSdobwSc8VQO6xFPk2Q3Hz4...)

[Dan Beam, 2017-02-06 17:37:43]

why can't we just change from

<paper-button>
  <content></content>
</paper-button>
<controlled-button>label</controlled-button>

to

<content></content>
<controlled-button>
  <paper-button>label</paper-button>
<controlled-button>

and use Polymer.dom API to go through the distributed children?

and pass i

[Dan Beam, 2017-02-06 17:38:47]

we could also just not use controlled-button for this

I also don't like the UI being proposed

[scottchen, 2017-02-06 19:43:06]

Description was changed from

==========
MD Settings: Allow controlled-button to be used with icons.

The goal is to "replace CHANGE button with outlink icon".

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

to

==========
MD Settings: Allow controlled-button to be used with icons.

The goal is to "replace CHANGE button with outlink icon".

-- Edit:
Rediscussed with designer, concluded on not moving forward with this change.

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