MD Settings: polish top-level bluetooth page

MD Settings: polish top-level bluetooth page

R=stevenjb@chromium.org
BUG=602538
Committed: https://crrev.com/58256ff511a983238942e5699965b6fa0d3a3ea9
Cr-Commit-Position: refs/heads/master@{#387220}

[Dan Beam, 2016-04-12 04:28:14]

before: http://imgur.com/a/Nbqk8
after: http://imgur.com/a/heRnV

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(left):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.html:27:
-webkit-margin-end: var(--checkbox-spacing);
this was subtly wrong (<paper-checkbox> is 16x16, <iron-icon> is 20x20)

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css
(left):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css:9:
padding-top: 5px;
this is all done now by .list-frame

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css:12:
#outer:hover:not([dropdown]) {
we're not doing :hover anywhere else yet, and if we do: it should be consistent

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css:29:
font-weight: bold;
bold looks bad (we don't actually have a weight for that in roboto.css), 500
looks good

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.css (left):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.css:6: color:
blue;
done with .primary-button

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.css:20:
-webkit-margin-end: 10px;
done with --iron-icon-spacing

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.css:31: margin:
10px 20px;
positioned better by .list-frame

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html
(right):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html:32: <div
class="layout horizontal center flex">
class="flex" fixes toggle position

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html:45:
<iron-collapse opened="[[canShowDeviceList_(bluetoothEnabled,
deviceListExpanded)]]">
if the list was open and the toggle disabled bluetooth, wacky things would
happen

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_shared_css.html (left):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_shared_css.html:242: .vertical-list >
div:first-of-type {
div was too restrictive

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_shared_css.html:19: font-size:
inherit;
probably fixes a lot of issues for us...

[stevenjb, 2016-04-12 15:58:49]

Thanks for picking this up!

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css
(left):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css:29:
font-weight: bold;
On 2016/04/12 04:28:14, Dan Beam wrote:
> bold looks bad (we don't actually have a weight for that in roboto.css), 500
> looks good

Do we / should we have a shared property we can use here now? This was basically
a placeholder before we had shared styles.

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css
(right):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css:21:
}
Shouldn't all stlying now be moved into the .html?

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.css
(right):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.css:19: }
Move to .html?

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html
(right):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html:45:
<iron-collapse opened="[[canShowDeviceList_(bluetoothEnabled,
deviceListExpanded)]]">
On 2016/04/12 04:28:14, Dan Beam wrote:
> if the list was open and the toggle disabled bluetooth, wacky things would
> happen

Michael has proposed splitting long computed properties lines in the Web UI
style guide draft
(https://docs.google.com/document/d/1G0t519y_cSW4etXVwruR145abZH0N5-9R7JyejzVo...).
We should discuss further there and migrate towards consistency.

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:203:
canShowDeviceList_: function() {
We never resolved this in the style guide, but we should at least be consistent
within a file. The rest of this file declares and uses the passed in properties.

FWIW at first look I thought this might be a bug since it appears that the
properties were not being passed to this.

Also, I just fixed a Polymer bug where only one out of 4 dependent properties
was being passed to a function. It was easy to spot the bug since the function
was using this.bar but not passing bar to the function.

[Dan Beam, 2016-04-12 19:51:02]

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css
(left):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css:29:
font-weight: bold;
On 2016/04/12 15:58:48, stevenjb wrote:
> On 2016/04/12 04:28:14, Dan Beam wrote:
> > bold looks bad (we don't actually have a weight for that in roboto.css), 500
> > looks good
> 
> Do we / should we have a shared property we can use here now? This was
basically
> a placeholder before we had shared styles.

yes, but it's ".selected" which doesn't quite apply here as it sounds mutually
exclusive (and more than 1 device can be connected at once, right?)

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css
(right):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css:21:
}
On 2016/04/12 15:58:48, stevenjb wrote:
> Shouldn't all stlying now be moved into the .html?

I don't think we have a fast and hard rule about that, but the current code uses
a deprecated syntax (<link type=css>) and I prefer in the .html, so Done!

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.css
(right):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.css:19: }
On 2016/04/12 15:58:48, stevenjb wrote:
> Move to .html?

bluetooth_add_device_dialog.html and bluetooth_pair_device_dialog.html both
import this file as well, BUT only bluetooth_add_device_dialog.html uses
.no-devices (which I'll fix up shortly).

tl;dr - Done.

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html
(right):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html:45:
<iron-collapse opened="[[canShowDeviceList_(bluetoothEnabled,
deviceListExpanded)]]">
On 2016/04/12 15:58:48, stevenjb wrote:
> On 2016/04/12 04:28:14, Dan Beam wrote:
> > if the list was open and the toggle disabled bluetooth, wacky things would
> > happen
> 
> Michael has proposed splitting long computed properties lines in the Web UI
> style guide draft
>
(https://docs.google.com/document/d/1G0t519y_cSW4etXVwruR145abZH0N5-9R7JyejzVo...).
> We should discuss further there and migrate towards consistency.

Done.

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:203:
canShowDeviceList_: function() {
On 2016/04/12 15:58:49, stevenjb wrote:
> We never resolved this in the style guide, but we should at least be
consistent
> within a file. The rest of this file declares and uses the passed in
properties.
>
> 
> FWIW at first look I thought this might be a bug since it appears that the
> properties were not being passed to this.
> 
> Also, I just fixed a Polymer bug where only one out of 4 dependent properties
> was being passed to a function. It was easy to spot the bug since the function
> was using this.bar but not passing bar to the function.

Done.

[stevenjb, 2016-04-13 16:18:42]

Sorry, thought I'd published this yesterday.
LGTM with or without CSS variable for active connection font weight.

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css
(left):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css:29:
font-weight: bold;
On 2016/04/12 19:51:02, Dan Beam wrote:
> On 2016/04/12 15:58:48, stevenjb wrote:
> > On 2016/04/12 04:28:14, Dan Beam wrote:
> > > bold looks bad (we don't actually have a weight for that in roboto.css),
500
> > > looks good
> > 
> > Do we / should we have a shared property we can use here now? This was
> basically
> > a placeholder before we had shared styles.
> 
> yes, but it's ".selected" which doesn't quite apply here as it sounds mutually
> exclusive (and more than 1 device can be connected at once, right?)

I was thinking of a CSS variable, e.g. var(--settings-item-activated). We will
want to share it with internet at least.

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css
(right):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css:21:
}
On 2016/04/12 19:51:02, Dan Beam wrote:
> On 2016/04/12 15:58:48, stevenjb wrote:
> > Shouldn't all stlying now be moved into the .html?
> 
> I don't think we have a fast and hard rule about that, but the current code
uses
> a deprecated syntax (<link type=css>) and I prefer in the .html, so Done!

My understanding from Michael was that <link type=css> is deprecated and there
is no viable alternative except including all styling in the html. I figure any
time we make non trivial CSS changes we should just do that, so thanks!

[Dan Beam, 2016-04-13 16:45:49]

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css
(left):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css:29:
font-weight: bold;
On 2016/04/13 16:18:42, stevenjb wrote:
> On 2016/04/12 19:51:02, Dan Beam wrote:
> > On 2016/04/12 15:58:48, stevenjb wrote:
> > > On 2016/04/12 04:28:14, Dan Beam wrote:
> > > > bold looks bad (we don't actually have a weight for that in roboto.css),
> 500
> > > > looks good
> > > 
> > > Do we / should we have a shared property we can use here now? This was
> > basically
> > > a placeholder before we had shared styles.
> > 
> > yes, but it's ".selected" which doesn't quite apply here as it sounds
mutually
> > exclusive (and more than 1 device can be connected at once, right?)
> 
> I was thinking of a CSS variable, e.g. var(--settings-item-activated). We will
> want to share it with internet at least.

i assume you mean @apply() unless that variable is just "500" for the
font-weight (which isn't really worth sharing, IMO).

i'll do that in a followup

[Dan Beam, 2016-04-13 16:45:54]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2016-04-13 16:46:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1875413002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1875413002/20001

[stevenjb, 2016-04-13 16:58:42]

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css
(left):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css:29:
font-weight: bold;
On 2016/04/13 16:45:49, Dan Beam wrote:
> On 2016/04/13 16:18:42, stevenjb wrote:
> > On 2016/04/12 19:51:02, Dan Beam wrote:
> > > On 2016/04/12 15:58:48, stevenjb wrote:
> > > > On 2016/04/12 04:28:14, Dan Beam wrote:
> > > > > bold looks bad (we don't actually have a weight for that in
roboto.css),
> > 500
> > > > > looks good
> > > > 
> > > > Do we / should we have a shared property we can use here now? This was
> > > basically
> > > > a placeholder before we had shared styles.
> > > 
> > > yes, but it's ".selected" which doesn't quite apply here as it sounds
> mutually
> > > exclusive (and more than 1 device can be connected at once, right?)
> > 
> > I was thinking of a CSS variable, e.g. var(--settings-item-activated). We
will
> > want to share it with internet at least.
> 
> i assume you mean @apply() unless that variable is just "500" for the
> font-weight (which isn't really worth sharing, IMO).
> 
> i'll do that in a followup

@apply is probably better, but given that it was unknown to me that bold/700 was
not a supported font weight in roboto but 500 is, it seems very much worth
sharing IMHO.

[Dan Beam, 2016-04-13 17:38:22]

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css
(left):

https://codereview.chromium.org/1875413002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.css:29:
font-weight: bold;
On 2016/04/13 16:58:42, stevenjb wrote:
> On 2016/04/13 16:45:49, Dan Beam wrote:
> > On 2016/04/13 16:18:42, stevenjb wrote:
> > > On 2016/04/12 19:51:02, Dan Beam wrote:
> > > > On 2016/04/12 15:58:48, stevenjb wrote:
> > > > > On 2016/04/12 04:28:14, Dan Beam wrote:
> > > > > > bold looks bad (we don't actually have a weight for that in
> roboto.css),
> > > 500
> > > > > > looks good
> > > > > 
> > > > > Do we / should we have a shared property we can use here now? This was
> > > > basically
> > > > > a placeholder before we had shared styles.
> > > > 
> > > > yes, but it's ".selected" which doesn't quite apply here as it sounds
> > mutually
> > > > exclusive (and more than 1 device can be connected at once, right?)
> > > 
> > > I was thinking of a CSS variable, e.g. var(--settings-item-activated). We
> will
> > > want to share it with internet at least.
> > 
> > i assume you mean @apply() unless that variable is just "500" for the
> > font-weight (which isn't really worth sharing, IMO).
> > 
> > i'll do that in a followup
> 
> @apply is probably better, but given that it was unknown to me that bold/700
was
> not a supported font weight in roboto but 500 is, it seems very much worth
> sharing IMHO.

actually, it's been on my todo list to potentially just change 500 to bold in
roboto.css

https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

[commit-bot: I haz the power, 2016-04-13 17:48:39]

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

[commit-bot: I haz the power, 2016-04-13 17:48:40]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dan Beam, 2016-04-14 01:52:21]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-04-14 01:52:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1875413002/#ps40001
(title: "test fixes")

[commit-bot: I haz the power, 2016-04-14 01:52:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1875413002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1875413002/40001

[commit-bot: I haz the power, 2016-04-14 03:02:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-14 03:03:59]

Description was changed from

==========
MD Settings: polish top-level bluetooth page

R=stevenjb@chromium.org
BUG=602538
==========

to

==========
MD Settings: polish top-level bluetooth page

R=stevenjb@chromium.org
BUG=602538

Committed: https://crrev.com/58256ff511a983238942e5699965b6fa0d3a3ea9
Cr-Commit-Position: refs/heads/master@{#387220}
==========

[commit-bot: I haz the power, 2016-04-14 03:04:01]

Patchset 3 (id:??) landed as
https://crrev.com/58256ff511a983238942e5699965b6fa0d3a3ea9
Cr-Commit-Position: refs/heads/master@{#387220}