Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2538653002:
bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. (Closed)
Descriptionbluetooth: Add sidebar and page manager for chrome://bluetooth-internals.
Adds Sidebar control to chrome://bluetooth-internals.
Adds PageManager to manage view switching.
Adds DevicesPage wrapper around DeviceTable for paging.
Changes style to user-facing WebUI standard.
Screenshots: https://goo.gl/photos/EKWMtQ2DBfF3m9WC8
BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/d316a0b2e72525b938de1bdc9d74ef5b6c3df00e
Cr-Commit-Position: refs/heads/master@{#437459}
Patch Set 1 #Patch Set 2 : Fix styles, change layout #Patch Set 3 : Fix const #
Total comments: 2
Patch Set 4 : Change interface styling, add hidden property to device table #Patch Set 5 : Fix promise case #Patch Set 6 : Fix header width #
Total comments: 12
Patch Set 7 : Rearrange HTML layout, change code structure #
Total comments: 50
Patch Set 8 : Fix styles, change sidebar class assignment, move page header #
Total comments: 33
Patch Set 9 : Remove extra argument on sidebar statement #Patch Set 10 : Move header, fix up sidebar, add hash-based navigation #Patch Set 11 : Remove extraneous css rules #Patch Set 12 : Remove more css rules #
Total comments: 6
Patch Set 13 : Fix fade in/fade out of overlay, shrink title margin #Patch Set 14 : Set timing function on sidebar content #
Total comments: 16
Patch Set 15 : Add CSS variables, fix sidebar JSDoc #Patch Set 16 : Move transform-duration var #
Total comments: 2
Patch Set 17 : Change rgb gray values to hex, change text-align values to start or end #Dependent Patchsets: Messages
Total messages: 53 (26 generated)
Description was changed from ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. BUG=651282 ========== to ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. Screenshots: https://goo.gl/photos/JhRd5CF7uQVApFLr7 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
mbrunson@chromium.org changed reviewers: + ortuno@chromium.org, scheib@chromium.org
Description was changed from ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. Screenshots: https://goo.gl/photos/JhRd5CF7uQVApFLr7 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. Adds DevicesView wrapper around DeviceTable for paging. Screenshots: https://goo.gl/photos/JhRd5CF7uQVApFLr7 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
LGTM with my novice webui eye. https://codereview.chromium.org/2538653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2538653002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:76: var deviceInfo = devices.getByAddress(address); can this fail, as previous promise block would?
Description was changed from ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. Adds DevicesView wrapper around DeviceTable for paging. Screenshots: https://goo.gl/photos/JhRd5CF7uQVApFLr7 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. Adds DevicesView wrapper around DeviceTable for paging. Changes style to user-facing WebUI standard. Screenshots: https://goo.gl/photos/EKWMtQ2DBfF3m9WC8 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Screenshots and style have been updated. https://codereview.chromium.org/2538653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2538653002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:76: var deviceInfo = devices.getByAddress(address); On 2016/11/29 04:33:44, scheib wrote: > can this fail, as previous promise block would? Yes. I'll check the response before running this. That should catch that case. Done.
https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:31: <div id="menu-btn"></div> menu-btn and overlay seem really out place semantically. I would encourage you to look at how polymer structures an app like this [1]. I looked a bit and a couple of things stand out: - The structure has more semantic meaning: header, content, and drawer. - They do some clever things with the ordering and positioning of the drawer and overlay that could simplify our css. - They don't use media queries. Have you considered using window.mediaMatches to set the classes? It would work similarly to iron-media-query [1] https://polymerelements.github.io/app-layout/templates/getting-started/ https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:35: <li class="selected" data-page-name="devices">Devices</li> Would it make sense to just set the title based on the selected page? That way you don't have to repeat the title on each page. https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:45: </div> https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Using_HTML_sections_a... Please use elements with more semantic meaning e.g. <aside> for the sidebar, <button> for the clickable elements, etc. https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:101: .then(setupPages) Is the order correct here? If there is any error getting devices we would be left with no UI. https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/devices_view.js (right): https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/devices_view.js:13: * Page for device table and associated controls. nit: Page that contains a device table and associated controls. https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/sidebar.js (right): https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:42: * Closes the sidebar. Only applies to mobile layouts. Can you be more specific? What do you mean when you say it only applies to mobile layouts?
https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:31: <div id="menu-btn"></div> On 2016/12/01 06:27:03, ortuno wrote: > menu-btn and overlay seem really out place semantically. I would encourage you > to look at how polymer structures an app like this [1]. I looked a bit and a > couple of things stand out: > > - The structure has more semantic meaning: header, content, and drawer. > - They do some clever things with the ordering and positioning of the drawer > and overlay that could simplify our css. > - They don't use media queries. Have you considered using window.mediaMatches > to set the classes? It would work similarly to iron-media-query > > [1] https://polymerelements.github.io/app-layout/templates/getting-started/ - I've added semantic tags. - Changing the ordering removed some CSS rules. - What's the advantage of window.matchMedia? We would still have the CSS classes to handle small screen widths in addition to new JS code to manage. Why not just let the browser handle it with media queries? https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:35: <li class="selected" data-page-name="devices">Devices</li> On 2016/12/01 06:27:03, ortuno wrote: > Would it make sense to just set the title based on the selected page? That way > you don't have to repeat the title on each page. The menu button is now a part of the page header. I've separated the Page concept from its contents. So, there is now a DevicesPage and a DevicesView. In the next patch, I'll make a generic Page superclass to handle setting up the header for each page. https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:45: </div> On 2016/12/01 06:27:03, ortuno wrote: > https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Using_HTML_sections_a... > > Please use elements with more semantic meaning e.g. <aside> for the sidebar, > <button> for the clickable elements, etc. Done. https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:101: .then(setupPages) On 2016/12/01 06:27:03, ortuno wrote: > Is the order correct here? If there is any error getting devices we would be > left with no UI. Yes. That's a possibility. I'll initialize the pages before the Promise chain runs. Done. https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/devices_view.js (right): https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/devices_view.js:13: * Page for device table and associated controls. On 2016/12/01 06:27:03, ortuno wrote: > nit: Page that contains a device table and associated controls. Done. https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/sidebar.js (right): https://codereview.chromium.org/2538653002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:42: * Closes the sidebar. Only applies to mobile layouts. On 2016/12/01 06:27:03, ortuno wrote: > Can you be more specific? What do you mean when you say it only applies to > mobile layouts? Done.
https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:20: margin-top: 21px; Some of these values seem pretty random. Any reason why we are not reusing the ones that about:inspect uses? https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:21: } There are two types of headers: Sidebar and Page. We have the following rule: #page-container header > h1 { margin: 0; padding: 21px 0 13px; } which means the only property from the first rule that applies is color. I think the following would be better: h1 { color: rgb(92, 97, 102); } .sidebar-content > header > h1 { -webkit-margin-start: 23px; margin-bottom: 1em; margin-top: 21px; } https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:46: } What about: /* Page container */ #page-container { margin-left: 155px; } @media screen and (max-width: 600px) { #page-container { margin-left: 0; } } /* Page content */ .page-content { padding: 24px 16px; } Does that miss anything? https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:49: #page-container header { I think we can simplify things a bit if we used flex and an image for the button: /* Page header */ #page-container header { background-image: linear-gradient( white, white 40%, rgba(255, 255, 255, 0.92)); border-bottom: 1px solid #eee; display: flex; align-items: center; padding-top: 8px; } #page-container header > h1 { /* TODO: Figure out correct left margin */ margin: 13px 0 13px 12px; } .menu-btn { background-color: white; /* TODO: figure out image */ background-image: url(../options/info.svg); background-size: cover; border: 0; display: none; height: 24px; /* TODO: Figure out correct margin */ margin-left: 12px; width: 24px; } @media screen and (max-width: 600px) { .menu-btn { display: block; } } https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:104: If we live #sidebar without "position: absolute" then all of its children will be positioned relative to the nearest ancestor who has position: absolute or position: relative in this case body. Since we don't want that please a rule for #sidebar: #sidebar { bottom: 0; left: 0; position: fixed; right: 0; top: 0; width: 155px; transition: visibility 0.2s ease; } @media screen and (max-width: 600px) { #sidebar { width: auto; visibility: hidden; } #sidebar.open { visibility: visible; } } https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:152: display: none; We are already changing the opacity so might as well use that to show/hide the sidebar: .overlay { background-color: rgba(0, 0, 0, .5); bottom: 0; left: 0; opacity: 0; position: absolute; right: 0; top: 0; transition: opacity 225ms; } @media screen and (max-width: 600px) { .open .overlay { opacity: 1; } } https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:155: position: fixed; If you add #sidebar { position: fixed; } then this can just be absolute. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:162: @media screen and (min-width: 601px) { I'm curious as to why you use the following pattern: @media screen and (min-width: x + 1) { } @media screen and (max-width: x) { } ? It makes it really easy to loose track of the values since you now have to keep track of 3 rule sets(In this case you are setting the cursor twice). I think keeping just two sets of rules (base and max-width) should be enough. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:169: cursor: default; Why change this depending on size? https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:176: left: -155px; Prefer to use transform when animating for two reasons: 1. transform operations are optimized by the compositor 2. top, right, bottom, left could affect layout so css has to recalculate the whole layout. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:182: line-height: 48px; Why do you change this in smaller screens? It looks really jumpy when you go from 600px -> 601px. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:185: .sidebar-content.open { We can save ourselves some js lines by adding the open class to 'aside' directly. Then our css rules would look like the following: .open .sidebar-content { /* ... */ } .open .overlay { /* ... */ } https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:45: devicesPage.pageDiv.addEventListener('inspectpressed', function() { Seems a bit weird and flaky to be adding these before devices, adapterBroker, etc. are defined. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/devices_page.js (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/devices_page.js:21: this.pageDiv.appendChild(document.importNode($('page-template').content, Could we just have a single header and change its contents when changing pages? Otherwise we would have to replicate this code on each page... https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/devices_page.js:34: cr.addSingletonGetter(DevicesPage); Why use a singleton? https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/devices_page.js:60: this.appendChild(this.deviceTable); I'm so sad that we can't use Custom Elements :( https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/sidebar.js (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:27: }, this); q: Why do you need the this here? https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:31: this.overlayDiv_.addEventListener('click', this.close.bind(this)); Oh cool. Could we do the same on line 26? https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:41: this.sidebarContent_.classList.remove('open'); You can just set the class in aside by changing some of the css rules. See bluetooth_internals.css for more details. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:48: open: function() { Could you set overflow: hidden; in body when the bar is opened and the resolution is < 600px? That way the scroll bars are not shown when the toolbar is opened. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:52: One of the reasons I thought using a listener in js would be better was for resetting the sidebar when the resolution changes. For example the following steps result in the bar staying opened: 1. Start with width < 600 2. Open sidebar. 3. Increase width to > 600 4. Decrease width to < 600 <--- Sidebar starts opened. Could we do that with media queries? Maybe a combination of the two?
https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:20: margin-top: 21px; On 2016/12/05 05:45:28, ortuno wrote: > Some of these values seem pretty random. Any reason why we are not reusing the > ones that about:inspect uses? I was using chrome://settings's CSS as a guide. Each internal page looks similar but they all have their own stylesheets, so the values and configuration aren't exactly the same. There is no one truth here unfortunately so I'm partially copying and partially making it mobile friendly. Some of these rules are overlapping and could be combined/removed. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:21: } On 2016/12/05 05:45:28, ortuno wrote: > There are two types of headers: Sidebar and Page. > > We have the following rule: > > #page-container header > h1 { > margin: 0; > padding: 21px 0 13px; > } > > which means the only property from the first rule that applies is color. I think > the following would be better: > > h1 { > color: rgb(92, 97, 102); > } > > .sidebar-content > header > h1 { > -webkit-margin-start: 23px; > margin-bottom: 1em; > margin-top: 21px; > } Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:46: } On 2016/12/05 05:45:28, ortuno wrote: > What about: > > /* Page container */ > #page-container { > margin-left: 155px; > } > > @media screen and (max-width: 600px) { > #page-container { > margin-left: 0; > } > } > > /* Page content */ > .page-content { > padding: 24px 16px; > } > > Does that miss anything? Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:49: #page-container header { On 2016/12/05 05:45:28, ortuno wrote: > I think we can simplify things a bit if we used flex and an image for the > button: > > /* Page header */ > #page-container header { > background-image: linear-gradient( > white, white 40%, rgba(255, 255, 255, 0.92)); > border-bottom: 1px solid #eee; > display: flex; > align-items: center; > padding-top: 8px; > } > > #page-container header > h1 { > /* TODO: Figure out correct left margin */ > margin: 13px 0 13px 12px; > } > > .menu-btn { > background-color: white; > /* TODO: figure out image */ > background-image: url(../options/info.svg); > background-size: cover; > border: 0; > display: none; > height: 24px; > /* TODO: Figure out correct margin */ > margin-left: 12px; > width: 24px; > } > > @media screen and (max-width: 600px) { > .menu-btn { > display: block; > } > } > Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:104: On 2016/12/05 05:45:28, ortuno wrote: > If we live #sidebar without "position: absolute" then all of its children will > be positioned relative to the nearest ancestor who has position: absolute or > position: relative in this case body. Since we don't want that please a rule for > #sidebar: > > #sidebar { > bottom: 0; > left: 0; > position: fixed; > right: 0; > top: 0; > width: 155px; > transition: visibility 0.2s ease; > } > > @media screen and (max-width: 600px) { > #sidebar { > width: auto; > visibility: hidden; > } > > #sidebar.open { > visibility: visible; > } > } Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:152: display: none; On 2016/12/05 05:45:27, ortuno wrote: > We are already changing the opacity so might as well use that to show/hide the > sidebar: > > .overlay { > background-color: rgba(0, 0, 0, .5); > bottom: 0; > left: 0; > opacity: 0; > position: absolute; > right: 0; > top: 0; > transition: opacity 225ms; > } > > @media screen and (max-width: 600px) { > .open .overlay { > opacity: 1; > } > } Opacity only changes the transparency. Since the overlay is on top of the entire page, it steals clicks from the main content. I set it to display:none so it doesn't do this when it's invisible. It would probably be better to use the visibility property and animate that. Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:155: position: fixed; On 2016/12/05 05:45:28, ortuno wrote: > If you add #sidebar { position: fixed; } then this can just be absolute. Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:162: @media screen and (min-width: 601px) { On 2016/12/05 05:45:28, ortuno wrote: > I'm curious as to why you use the following pattern: > > @media screen and (min-width: x + 1) { > } > > @media screen and (max-width: x) { > } > > ? > > It makes it really easy to loose track of the values since you now have to keep > track of 3 rule sets(In this case you are setting the cursor twice). I think > keeping just two sets of rules (base and max-width) should be enough. Yeah. I originally wanted to make things explicit between what's shared, large screen only, and small screen only. But that pattern doesn't make much sense here since I'm only addressing two configurations. I'll consolidate things here. Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:169: cursor: default; On 2016/12/05 05:45:27, ortuno wrote: > Why change this depending on size? I don't think I need this anymore actually. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:176: left: -155px; On 2016/12/05 05:45:28, ortuno wrote: > Prefer to use transform when animating for two reasons: > > 1. transform operations are optimized by the compositor > 2. top, right, bottom, left could affect layout so css has to recalculate the > whole layout. Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:182: line-height: 48px; On 2016/12/05 05:45:27, ortuno wrote: > Why do you change this in smaller screens? It looks really jumpy when you go > from 600px -> 601px. The area for the buttons should be bigger to compensate for the comparatively smaller size of the buttons on devices with smaller screens. This is the recommended value for the size of touch targets on mobile devices. https://material.google.com/layout/metrics-keylines.html#metrics-keylines-tou... Keeping the 48px size makes the sidebar buttons very large on desktop monitors, so I changed it just for smaller screens. Ideally, there would be a third type of layout for tablets and mobile devices in landscape to minimize the changes between layouts, but I will save this for a later patch if it becomes an issue. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:185: .sidebar-content.open { On 2016/12/05 05:45:28, ortuno wrote: > We can save ourselves some js lines by adding the open class to 'aside' > directly. Then our css rules would look like the following: > > .open .sidebar-content { > /* ... */ > } > > .open .overlay { > /* ... */ > } Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:45: devicesPage.pageDiv.addEventListener('inspectpressed', function() { On 2016/12/05 05:45:28, ortuno wrote: > Seems a bit weird and flaky to be adding these before devices, adapterBroker, > etc. are defined. I moved this event listener to the setupDeviceCollection. I'll rename that to setupDeviceSystem or something since it's attaching listeners for device collection and doing various other Device-related things. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/devices_page.js (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/devices_page.js:21: this.pageDiv.appendChild(document.importNode($('page-template').content, On 2016/12/05 05:45:28, ortuno wrote: > Could we just have a single header and change its contents when changing pages? > Otherwise we would have to replicate this code on each page... Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/devices_page.js:34: cr.addSingletonGetter(DevicesPage); On 2016/12/05 05:45:28, ortuno wrote: > Why use a singleton? Other internal pages that use PageManager tend to have singletons for all the pages since there will always only be one instance of the page. I would be fine with this not being a singleton but it seemed like the preferred pattern. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/devices_page.js:60: this.appendChild(this.deviceTable); On 2016/12/05 05:45:28, ortuno wrote: > I'm so sad that we can't use Custom Elements :( :'( https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/sidebar.js (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:27: }, this); On 2016/12/05 05:45:28, ortuno wrote: > q: Why do you need the this here? The this object in the forEach is the Window object so I had to override that. It's the same as using the bind function. I've done this in multiple places with forEach loops. See device_table.js (96,97) and bluetooth_internals.js (36) https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:31: this.overlayDiv_.addEventListener('click', this.close.bind(this)); On 2016/12/05 05:45:28, ortuno wrote: > Oh cool. Could we do the same on line 26? Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:41: this.sidebarContent_.classList.remove('open'); On 2016/12/05 05:45:28, ortuno wrote: > You can just set the class in aside by changing some of the css rules. See > bluetooth_internals.css for more details. Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:48: open: function() { On 2016/12/05 05:45:28, ortuno wrote: > Could you set overflow: hidden; in body when the bar is opened and the > resolution is < 600px? That way the scroll bars are not shown when the toolbar > is opened. Done. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:52: On 2016/12/05 05:45:28, ortuno wrote: > One of the reasons I thought using a listener in js would be better was for > resetting the sidebar when the resolution changes. For example the following > steps result in the bar staying opened: > > 1. Start with width < 600 > 2. Open sidebar. > 3. Increase width to > 600 > 4. Decrease width to < 600 <--- Sidebar starts opened. > > Could we do that with media queries? Maybe a combination of the two? Oh. I see. In that case, the listener would be good because it's managing the state of the component based on the screen size. Done.
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:20: margin-top: 21px; On 2016/12/06 at 00:25:45, mbrunson wrote: > On 2016/12/05 05:45:28, ortuno wrote: > > Some of these values seem pretty random. Any reason why we are not reusing the > > ones that about:inspect uses? > > I was using chrome://settings's CSS as a guide. Each internal page looks similar but they all have their own stylesheets, so the values and configuration aren't exactly the same. There is no one truth here unfortunately so I'm partially copying and partially making it mobile friendly. > > Some of these rules are overlapping and could be combined/removed. :( hopefully we share more with polymer. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:152: display: none; On 2016/12/06 at 00:25:45, mbrunson wrote: > On 2016/12/05 05:45:27, ortuno wrote: > > We are already changing the opacity so might as well use that to show/hide the > > sidebar: > > > > .overlay { > > background-color: rgba(0, 0, 0, .5); > > bottom: 0; > > left: 0; > > opacity: 0; > > position: absolute; > > right: 0; > > top: 0; > > transition: opacity 225ms; > > } > > > > @media screen and (max-width: 600px) { > > .open .overlay { > > opacity: 1; > > } > > } > > Opacity only changes the transparency. Since the overlay is on top of the entire page, it steals clicks from the main content. I set it to display:none so it doesn't do this when it's invisible. > > It would probably be better to use the visibility property and animate that. Done. Right, but now we set visibility on #sidebar, so I think we can still use opacity here. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:182: line-height: 48px; On 2016/12/06 at 00:25:45, mbrunson wrote: > On 2016/12/05 05:45:27, ortuno wrote: > > Why do you change this in smaller screens? It looks really jumpy when you go > > from 600px -> 601px. > > The area for the buttons should be bigger to compensate for the comparatively smaller size of the buttons on devices with smaller screens. This is the recommended value for the size of touch targets on mobile devices. > > https://material.google.com/layout/metrics-keylines.html#metrics-keylines-tou... > > Keeping the 48px size makes the sidebar buttons very large on desktop monitors, so I changed it just for smaller screens. Ideally, there would be a third type of layout for tablets and mobile devices in landscape to minimize the changes between layouts, but I will save this for a later patch if it becomes an issue. I see. What about 40px? Seems like an acceptable size based on densed lists in [1] and doesn't look too big on bigger screenss [1] https://material.google.com/components/lists.html#lists-specs https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/sidebar.js (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:27: }, this); On 2016/12/06 at 00:25:46, mbrunson wrote: > On 2016/12/05 05:45:28, ortuno wrote: > > q: Why do you need the this here? > > The this object in the forEach is the Window object so I had to override that. It's the same as using the bind function. I've done this in multiple places with forEach loops. See device_table.js (96,97) and bluetooth_internals.js (36) Ah got it. Thanks for the explanation. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:24: max-height: calc(100% - 57px); /* Subtract total height of page header */ Why do we want to always keep the header up top? We end up with less pixels for content :p If we really wanted to do it right we would hide/show the sidebar as you scroll but I think that's too much for this page. If you decide to keep the header up top I would use position sticky instead: .page-header { position: sticky; top: 0; } Then this would become: #page-container { margin-left: 155px; } https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:57: background-image: url(../../../../ui/webui/resources/images/vr_overflow.svg); Per spec, we want the hamburger icon. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:61: height: 48px; I think the icon size should be 24px but the target should be 48px https://material.google.com/layout/metrics-keylines.html#metrics-keylines-tou... https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:87: transition: visibility 200ms ease; optional: I put in ease as a placeholder (ease is actually the default value). Feel free to change to more fancy cubic bezier to match other animations. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:125: line-height: 1.417em; Why use line-height instead of height? Also this number looks really random O.o https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:130: .sidebar-content li { How hard would it be to apply the style to the button directly? Or maybe a combination of both where most of the style is in the button but the border is in <li>. - Seems a bit weird to style the <li> when in reality it is the <button> that you are pressing. - When you tab around only the name is highlighted. - When you click on the button the name is highlighted but when you click on the li nothing is highlighted. - We could save some styling rules e.g. cursor: pointer. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:161: transform: translateX(-155px); So Polymer does two things differently. They: 1. use translate3d 2. use -100% When looking into it I found out that 1. translate3d is accelerated by the GPU but translate is not. So we should use translate3d. [1] 2. If you use percentages in translate it won't be run on the compositor D: So -155px is good. [2] (Consider staring http://crbug.com/230229 so we can get some of this information in dev tools :)) [1] Start of the relevant section: https://youtu.be/q_O9_C2ZjoA?t=9m55s How to trigger HW accelerated animations: https://youtu.be/q_O9_C2ZjoA?t=12m10s [2] crbug.com/389359 https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:171: transform: translateX(0); Same here, use translate3d. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:173: transition-timing-function: cubic-bezier(0.4, 0, 0.6, 1); Curious as to why you split this. The style guide says to use shorthand properties when possible: https://google.github.io/styleguide/htmlcssguide.xml#Shorthand_Properties https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:178: transition-timing-function: cubic-bezier(0.4, 0, 0.6, 1); If you only have the transition property here there is no animation when going from .open .overlay to just .overlay. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:31: <header class="page-header"> q: Why is this outside of page container? If we keep it outside then we need to set two rules one for the header and one for the container. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:33: <h1 class="page-title">Devices</h1> I can't seem to find where this is being updated? Would it make sense to use the updateTitle function of the observer? https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:87: var sidebar = new window.sidebar.Sidebar('sidebar', 'overlay'); optional nit: I think passing the element would make this clearer new window.sidebar.Sidebar($('sidebar')); https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:91: var devicesPage = DevicesPage.getInstance(); I think we should ask someone from WebUI if there are any technical reasons why we have to use the singleton pattern. IMO explicitly initializing the page here would be clearer. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/devices_page.js (right): https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/devices_page.js:45: * An element that contains a device table and its associated controls. Why not just add the table directly to pageDiv? https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/devices_page.js:54: decorate: function() { q: Who calls this?
FYI: There is a performance issue with the current table setup when a large number of devices (>50) is present. There's a simple fix for this that I'm adding in a separate patch. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:152: display: none; On 2016/12/06 10:16:04, ortuno wrote: > On 2016/12/06 at 00:25:45, mbrunson wrote: > > On 2016/12/05 05:45:27, ortuno wrote: > > > We are already changing the opacity so might as well use that to show/hide > the > > > sidebar: > > > > > > .overlay { > > > background-color: rgba(0, 0, 0, .5); > > > bottom: 0; > > > left: 0; > > > opacity: 0; > > > position: absolute; > > > right: 0; > > > top: 0; > > > transition: opacity 225ms; > > > } > > > > > > @media screen and (max-width: 600px) { > > > .open .overlay { > > > opacity: 1; > > > } > > > } > > > > Opacity only changes the transparency. Since the overlay is on top of the > entire page, it steals clicks from the main content. I set it to display:none so > it doesn't do this when it's invisible. > > > > It would probably be better to use the visibility property and animate that. > Done. > > Right, but now we set visibility on #sidebar, so I think we can still use > opacity here. Yes. But the sidebar is visible by default on large screens so it covers the sidebar content and prevents it from capturing mouse events since it's absolutely positioned. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:182: line-height: 48px; On 2016/12/06 10:16:04, ortuno wrote: > On 2016/12/06 at 00:25:45, mbrunson wrote: > > On 2016/12/05 05:45:27, ortuno wrote: > > > Why do you change this in smaller screens? It looks really jumpy when you go > > > from 600px -> 601px. > > > > The area for the buttons should be bigger to compensate for the comparatively > smaller size of the buttons on devices with smaller screens. This is the > recommended value for the size of touch targets on mobile devices. > > > > > https://material.google.com/layout/metrics-keylines.html#metrics-keylines-tou... > > > > Keeping the 48px size makes the sidebar buttons very large on desktop > monitors, so I changed it just for smaller screens. Ideally, there would be a > third type of layout for tablets and mobile devices in landscape to minimize the > changes between layouts, but I will save this for a later patch if it becomes an > issue. > > I see. What about 40px? Seems like an acceptable size based on densed lists in > [1] and doesn't look too big on bigger screenss > > [1] https://material.google.com/components/lists.html#lists-specs Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:24: max-height: calc(100% - 57px); /* Subtract total height of page header */ On 2016/12/06 10:16:05, ortuno wrote: > Why do we want to always keep the header up top? We end up with less pixels for > content :p > > If we really wanted to do it right we would hide/show the sidebar as you scroll > but I think that's too much for this page. If you decide to keep the header up > top I would use position sticky instead: > > .page-header { > position: sticky; > top: 0; > } > > Then this would become: > > #page-container { > margin-left: 155px; > } Yeah. That's much better. Hiding and showing it like Android apps do would be great since I was planning to put buttons/page-wide controls in the header. Nobody would want to scroll through long tables and have to scroll back to the top to access a button to start a scan, refresh the device, etc. especially on mobile devices. So, I just fixed it to the top to keep it simple. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:57: background-image: url(../../../../ui/webui/resources/images/vr_overflow.svg); On 2016/12/06 10:16:04, ortuno wrote: > Per spec, we want the hamburger icon. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:61: height: 48px; On 2016/12/06 10:16:05, ortuno wrote: > I think the icon size should be 24px but the target should be 48px > > https://material.google.com/layout/metrics-keylines.html#metrics-keylines-tou... Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:87: transition: visibility 200ms ease; On 2016/12/06 10:16:04, ortuno wrote: > optional: I put in ease as a placeholder (ease is actually the default value). > Feel free to change to more fancy cubic bezier to match other animations. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:125: line-height: 1.417em; On 2016/12/06 10:16:05, ortuno wrote: > Why use line-height instead of height? Also this number looks really random O.o Yes. It is really random. But that's the value chrome://settings uses for its button size in its sidebar. ¯\_(ツ)_/¯ I'm changing this to 40px as suggested in another comment. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:130: .sidebar-content li { On 2016/12/06 10:16:05, ortuno wrote: > How hard would it be to apply the style to the button directly? Or maybe a > combination of both where most of the style is in the button but the border is > in <li>. > - Seems a bit weird to style the <li> when in reality it is the <button> that > you are pressing. > - When you tab around only the name is highlighted. > - When you click on the button the name is highlighted but when you click on > the li nothing is highlighted. > - We could save some styling rules e.g. cursor: pointer. I basically removed all styling from the li. The button has the click event and the styling now. I had to add some padding around to keep the text from jumping when the border is added, but, other than that, it should be the same. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:161: transform: translateX(-155px); On 2016/12/06 10:16:05, ortuno wrote: > So Polymer does two things differently. They: > > 1. use translate3d > 2. use -100% > > When looking into it I found out that > > 1. translate3d is accelerated by the GPU but translate is not. So we should use > translate3d. [1] > 2. If you use percentages in translate it won't be run on the compositor D: So > -155px is good. [2] > > (Consider staring http://crbug.com/230229 so we can get some of this information > in dev tools :)) > > [1] Start of the relevant section: https://youtu.be/q_O9_C2ZjoA?t=9m55s > How to trigger HW accelerated animations: > https://youtu.be/q_O9_C2ZjoA?t=12m10s > [2] crbug.com/389359 Yikes! Caveats all over the place D: Ok. I'll use translate3d with pixels from now on. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:171: transform: translateX(0); On 2016/12/06 10:16:05, ortuno wrote: > Same here, use translate3d. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:173: transition-timing-function: cubic-bezier(0.4, 0, 0.6, 1); On 2016/12/06 10:16:05, ortuno wrote: > Curious as to why you split this. The style guide says to use shorthand > properties when possible: > https://google.github.io/styleguide/htmlcssguide.xml#Shorthand_Properties Ah ok. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:178: transition-timing-function: cubic-bezier(0.4, 0, 0.6, 1); On 2016/12/06 10:16:05, ortuno wrote: > If you only have the transition property here there is no animation when going > from .open .overlay to just .overlay. I've moved this to .overlay. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:31: <header class="page-header"> On 2016/12/06 10:16:05, ortuno wrote: > q: Why is this outside of page container? If we keep it outside then we need to > set two rules one for the header and one for the container. Moved. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:33: <h1 class="page-title">Devices</h1> On 2016/12/06 10:16:05, ortuno wrote: > I can't seem to find where this is being updated? Would it make sense to use the > updateTitle function of the observer? The change in structure has thrown a few things off here. I've added a PageObserver to bluetooth_internals.js to handle this. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:87: var sidebar = new window.sidebar.Sidebar('sidebar', 'overlay'); On 2016/12/06 10:16:05, ortuno wrote: > optional nit: I think passing the element would make this clearer new > window.sidebar.Sidebar($('sidebar')); Ah. I agree. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:91: var devicesPage = DevicesPage.getInstance(); On 2016/12/06 10:16:05, ortuno wrote: > I think we should ask someone from WebUI if there are any technical reasons why > we have to use the singleton pattern. IMO explicitly initializing the page here > would be clearer. There doesn't seem to be a technical reason. It works fine either way and there are a few places that just use "new". I'll stray away from the singleton pattern for now. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/devices_page.js (right): https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/devices_page.js:45: * An element that contains a device table and its associated controls. On 2016/12/06 10:16:05, ortuno wrote: > Why not just add the table directly to pageDiv? This is a holdover from adding the PageManager. I wanted to separate the view-related stuff from the PageManager system. Each Page object would inherit from a superclass that centralized the creation of the view-specific header. Since the header is now shared, this separation isn't needed. I'll merge DevicesView into DevicesPage. Done. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/devices_page.js:54: decorate: function() { On 2016/12/06 10:16:05, ortuno wrote: > q: Who calls this? It's called in the cr library when the constructor is run. https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui.js?rcl=0&l=96
Almost there! Just one small issue and a question. https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:152: display: none; On 2016/12/07 at 01:12:13, mbrunson wrote: > On 2016/12/06 10:16:04, ortuno wrote: > > On 2016/12/06 at 00:25:45, mbrunson wrote: > > > On 2016/12/05 05:45:27, ortuno wrote: > > > > We are already changing the opacity so might as well use that to show/hide > > the > > > > sidebar: > > > > > > > > .overlay { > > > > background-color: rgba(0, 0, 0, .5); > > > > bottom: 0; > > > > left: 0; > > > > opacity: 0; > > > > position: absolute; > > > > right: 0; > > > > top: 0; > > > > transition: opacity 225ms; > > > > } > > > > > > > > @media screen and (max-width: 600px) { > > > > .open .overlay { > > > > opacity: 1; > > > > } > > > > } > > > > > > Opacity only changes the transparency. Since the overlay is on top of the > > entire page, it steals clicks from the main content. I set it to display:none so > > it doesn't do this when it's invisible. > > > > > > It would probably be better to use the visibility property and animate that. > > Done. > > > > Right, but now we set visibility on #sidebar, so I think we can still use > > opacity here. > > Yes. But the sidebar is visible by default on large screens so it covers the sidebar content and prevents it from capturing mouse events since it's absolutely positioned. Ah right. On my version I had sidebar-content as position absolute as well. Note that visibility doesn't have intermediate steps because it's discrete. It's either hidden or visible. If you look at the animation you can see that it doesn't fade out, it just disappears after a bit. https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:130: .sidebar-content li { On 2016/12/07 at 01:12:13, mbrunson wrote: > On 2016/12/06 10:16:05, ortuno wrote: > > How hard would it be to apply the style to the button directly? Or maybe a > > combination of both where most of the style is in the button but the border is > > in <li>. > > - Seems a bit weird to style the <li> when in reality it is the <button> that > > you are pressing. > > - When you tab around only the name is highlighted. > > - When you click on the button the name is highlighted but when you click on > > the li nothing is highlighted. > > - We could save some styling rules e.g. cursor: pointer. > > I basically removed all styling from the li. The button has the click event and the styling now. I had to add some padding around to keep the text from jumping when the border is added, but, other than that, it should be the same. Done. Good job! It looks really nice. https://codereview.chromium.org/2538653002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/220001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:66: margin: 13px 0 13px 28px; Q: Why such a big margin on the left? https://codereview.chromium.org/2538653002/diff/220001/ui/webui/resources/ima... File ui/webui/resources/images/menu.svg (right): https://codereview.chromium.org/2538653002/diff/220001/ui/webui/resources/ima... ui/webui/resources/images/menu.svg:1: <svg fill="#000000" height="24" viewBox="0 0 24 24" width="24" xmlns="http://www.w3.org/2000/svg"> Surprised that none of the images have copyright O.O
https://codereview.chromium.org/2538653002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/220001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:50: background-image: url(../../../../ui/webui/resources/images/menu.svg); nit: I think we use the grey icons not the black ones.
https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:152: display: none; On 2016/12/07 02:58:17, ortuno wrote: > On 2016/12/07 at 01:12:13, mbrunson wrote: > > On 2016/12/06 10:16:04, ortuno wrote: > > > On 2016/12/06 at 00:25:45, mbrunson wrote: > > > > On 2016/12/05 05:45:27, ortuno wrote: > > > > > We are already changing the opacity so might as well use that to > show/hide > > > the > > > > > sidebar: > > > > > > > > > > .overlay { > > > > > background-color: rgba(0, 0, 0, .5); > > > > > bottom: 0; > > > > > left: 0; > > > > > opacity: 0; > > > > > position: absolute; > > > > > right: 0; > > > > > top: 0; > > > > > transition: opacity 225ms; > > > > > } > > > > > > > > > > @media screen and (max-width: 600px) { > > > > > .open .overlay { > > > > > opacity: 1; > > > > > } > > > > > } > > > > > > > > Opacity only changes the transparency. Since the overlay is on top of the > > > entire page, it steals clicks from the main content. I set it to > display:none so > > > it doesn't do this when it's invisible. > > > > > > > > It would probably be better to use the visibility property and animate > that. > > > Done. > > > > > > Right, but now we set visibility on #sidebar, so I think we can still use > > > opacity here. > > > > Yes. But the sidebar is visible by default on large screens so it covers the > sidebar content and prevents it from capturing mouse events since it's > absolutely positioned. > > Ah right. On my version I had sidebar-content as position absolute as well. Note > that visibility doesn't have intermediate steps because it's discrete. It's > either hidden or visible. If you look at the animation you can see that it > doesn't fade out, it just disappears after a bit. Ah I see. I added opacity as well and just set the visibility transition duration such that it's hidden when opacity hits 0. Done. https://codereview.chromium.org/2538653002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/220001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:50: background-image: url(../../../../ui/webui/resources/images/menu.svg); On 2016/12/07 03:01:16, ortuno wrote: > nit: I think we use the grey icons not the black ones. Done. https://codereview.chromium.org/2538653002/diff/220001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:66: margin: 13px 0 13px 28px; On 2016/12/07 02:58:17, ortuno wrote: > Q: Why such a big margin on the left? This should be 24px actually. My math was off a little. The title is recommended to be 72 pixels from the left edge of the screen. https://material.google.com/layout/structure.html#structure-app-bar https://codereview.chromium.org/2538653002/diff/220001/ui/webui/resources/ima... File ui/webui/resources/images/menu.svg (right): https://codereview.chromium.org/2538653002/diff/220001/ui/webui/resources/ima... ui/webui/resources/images/menu.svg:1: <svg fill="#000000" height="24" viewBox="0 0 24 24" width="24" xmlns="http://www.w3.org/2000/svg"> On 2016/12/07 02:58:17, ortuno wrote: > Surprised that none of the images have copyright O.O Yeah. I can understand the open source ones not having it, but even the product logo SVGs don't.
lgtm with transition fix
mbrunson@chromium.org changed reviewers: + dpapad@chromium.org
dpapad@chromium.org: WebUI OWNERS review, please.
https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:18: margin-left: 155px; Have you this UI in RTL? These should be -webkit-margin-start instead (so that they are properly flipped, see examples https://cs.chromium.org/search/?q=webkit-margin-start+lang:css&sq=package:chr.... https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:40: position: sticky; Interesting! https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:152: transition: transform 195ms; Could you use CSS vars where possible to avoid repeating the same literals? See https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_variables. https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:42: <h1>Bluetooth Internals</h1> Note: Perhaps worth filing a bug to track the work needed for localizing bluetooth-internals strings. Currently all strings are hardcoded to English. https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/sidebar.js (right): https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:16: * @param {string} sidebarDiv The div corresponding to the sidebar. This is declared as a string here, and as an Element at line 21. https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:72: * @param {!Event} event Nit: Usually @private is the last annotation. https://codereview.chromium.org/2538653002/diff/260001/ui/webui/resources/htm... File ui/webui/resources/html/cr/ui/page_manager/page_manager.html (right): https://codereview.chromium.org/2538653002/diff/260001/ui/webui/resources/htm... ui/webui/resources/html/cr/ui/page_manager/page_manager.html:1: <script src="chrome://resources/js/cr/ui/page_manager/page_manager.js"></script> Do you need to import those files via HTML? Why not just import via <script> (example https://cs.chromium.org/chromium/src/chrome/browser/resources/help/help.html?...).
https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:18: margin-left: 155px; On 2016/12/07 19:33:20, dpapad wrote: > Have you this UI in RTL? These should be -webkit-margin-start instead (so that > they are properly flipped, see examples > https://cs.chromium.org/search/?q=webkit-margin-start+lang:css&sq=package:chr.... Added -webkit-margin-start, -webkit-border-start, and -webkit-padding-start where possible. What's the usual way people view internal pages in RTL? Extensions don't work on internal pages, right? Do I just change the language? https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:40: position: sticky; On 2016/12/07 19:33:20, dpapad wrote: > Interesting! And convenient! :D Looks like it was just enabled by default in Chrome 56. https://www.chromestatus.com/feature/6190250464378880 https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:152: transition: transform 195ms; On 2016/12/07 19:33:20, dpapad wrote: > Could you use CSS vars where possible to avoid repeating the same literals? See > https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_variables. Oh. That's so convenient! I wish I knew this existed earlier. Done. https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:42: <h1>Bluetooth Internals</h1> On 2016/12/07 19:33:20, dpapad wrote: > Note: Perhaps worth filing a bug to track the work needed for localizing > bluetooth-internals strings. Currently all strings are hardcoded to English. Line 4 - crbug.com/658814 I have a bug. I haven't started it yet. https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/sidebar.js (right): https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:16: * @param {string} sidebarDiv The div corresponding to the sidebar. On 2016/12/07 19:33:20, dpapad wrote: > This is declared as a string here, and as an Element at line 21. Done. https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/sidebar.js:72: * @param {!Event} event On 2016/12/07 19:33:20, dpapad wrote: > Nit: Usually @private is the last annotation. Done. https://codereview.chromium.org/2538653002/diff/260001/ui/webui/resources/htm... File ui/webui/resources/html/cr/ui/page_manager/page_manager.html (right): https://codereview.chromium.org/2538653002/diff/260001/ui/webui/resources/htm... ui/webui/resources/html/cr/ui/page_manager/page_manager.html:1: <script src="chrome://resources/js/cr/ui/page_manager/page_manager.js"></script> On 2016/12/07 19:33:20, dpapad wrote: > Do you need to import those files via HTML? Why not just import via <script> > (example > https://cs.chromium.org/chromium/src/chrome/browser/resources/help/help.html?...). I've been told to prefer html imports to prevent duplicating script loading. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc...
LGTM https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:18: margin-left: 155px; On 2016/12/07 at 21:15:56, mbrunson wrote: > On 2016/12/07 19:33:20, dpapad wrote: > > Have you this UI in RTL? These should be -webkit-margin-start instead (so that > > they are properly flipped, see examples > > https://cs.chromium.org/search/?q=webkit-margin-start+lang:css&sq=package:chr.... > > Added -webkit-margin-start, -webkit-border-start, and -webkit-padding-start where possible. > > What's the usual way people view internal pages in RTL? Extensions don't work on internal pages, right? Do I just change the language? I run it as follows LANGUAGE=ar ./out/<your_out_folder>/chrome --user-data-dir=/tmp/debug_chrome --no-first-run which launches Chrome in Arabic (and RTL). https://codereview.chromium.org/2538653002/diff/260001/ui/webui/resources/htm... File ui/webui/resources/html/cr/ui/page_manager/page_manager.html (right): https://codereview.chromium.org/2538653002/diff/260001/ui/webui/resources/htm... ui/webui/resources/html/cr/ui/page_manager/page_manager.html:1: <script src="chrome://resources/js/cr/ui/page_manager/page_manager.js"></script> > I've been told to prefer html imports to prevent duplicating script loading. > > https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... I see. I guess we could be auto-generating those dummy HTML files instead of checking them in (I'll put it in my potential TODO improvements list). https://codereview.chromium.org/2538653002/diff/300001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:190: background-color: #BDBDBD; Nit: The styleguide says "Hex is acceptable for shades of grey." (https://www.chromium.org/developers/web-development-style-guide). Let's use either rgb() or hex for all gray values in this file (for example lines 180, 185, 190), for consistency.
https://codereview.chromium.org/2538653002/diff/300001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2538653002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:190: background-color: #BDBDBD; On 2016/12/07 21:34:23, dpapad wrote: > Nit: The styleguide says "Hex is acceptable for shades of grey." > (https://www.chromium.org/developers/web-development-style-guide). Let's use > either rgb() or hex for all gray values in this file (for example lines 180, > 185, 190), for consistency. Done.
The CQ bit was checked by mbrunson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, ortuno@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2538653002/#ps320001 (title: "Change rgb gray values to hex, change text-align values to start or end")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by mbrunson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Description was changed from ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. Adds DevicesView wrapper around DeviceTable for paging. Changes style to user-facing WebUI standard. Screenshots: https://goo.gl/photos/EKWMtQ2DBfF3m9WC8 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. Adds DevicesPage wrapper around DeviceTable for paging. Changes style to user-facing WebUI standard. Screenshots: https://goo.gl/photos/EKWMtQ2DBfF3m9WC8 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by mbrunson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mbrunson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1481251753368660,
"parent_rev": "fdede02d198a3b30da32ee176540e809de34fdc1", "commit_rev":
"409bf2e0a51f913611b97bbaf15e5d7f19e75cbd"}
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. Adds DevicesPage wrapper around DeviceTable for paging. Changes style to user-facing WebUI standard. Screenshots: https://goo.gl/photos/EKWMtQ2DBfF3m9WC8 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. Adds DevicesPage wrapper around DeviceTable for paging. Changes style to user-facing WebUI standard. Screenshots: https://goo.gl/photos/EKWMtQ2DBfF3m9WC8 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. Adds DevicesPage wrapper around DeviceTable for paging. Changes style to user-facing WebUI standard. Screenshots: https://goo.gl/photos/EKWMtQ2DBfF3m9WC8 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add sidebar and page manager for chrome://bluetooth-internals. Adds Sidebar control to chrome://bluetooth-internals. Adds PageManager to manage view switching. Adds DevicesPage wrapper around DeviceTable for paging. Changes style to user-facing WebUI standard. Screenshots: https://goo.gl/photos/EKWMtQ2DBfF3m9WC8 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/d316a0b2e72525b938de1bdc9d74ef5b6c3df00e Cr-Commit-Position: refs/heads/master@{#437459} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/d316a0b2e72525b938de1bdc9d74ef5b6c3df00e Cr-Commit-Position: refs/heads/master@{#437459} |
