Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
981203007:
Scaffold for MD-Settings. (Closed)
|
Created:
5 years, 9 months ago by michaelpg Modified:
5 years, 9 months ago CC:
chromium-reviews, khorimoto+watch-md-settings_chromium.org, michaelpg+watch-polymer_chromium.org, michaelpg+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, orenb+watch-md-settings_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-closure_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionScaffold for MD-Settings.
Design doc: https://docs.google.com/a/google.com/document/d/1AtnJaHxCtohBG377EHWjuiKMfAy05et40GxKx07dcCQ/edit?usp=sharing
This CL creates a scaffold with a menu and a container for Settings pages.
There's obviously still work to be done in creating and styling components, but the goal of this CL
is to stay simple and provide a framework for iteration.
To keep this small I've uploaded an add-on CL:
https://codereview.chromium.org/986843002/
The add-on CL actually adds these resources to Chrome, updates a11y-page to fit in the scaffold
and adds a placeholder page, and points chrome://md-settings to the new scaffold. The delta
between patch sets 5 and 6 in that add-on CL show what the next CL would be after this one.
BUG=464979
R=jhawkins@chromium.org,stevenjb@chromium.org
Committed: https://crrev.com/8be27dccec415091458637d80a46151a0ba3038f
Cr-Commit-Position: refs/heads/master@{#320436}
Patch Set 1 : Work in progress #Patch Set 2 : #Patch Set 3 : remove help #
Total comments: 65
Patch Set 4 : Feedback. Update core-selected-pages usage. #
Total comments: 19
Patch Set 5 : some fixes #
Total comments: 30
Patch Set 6 : #Patch Set 7 : icon fix #Patch Set 8 : license #Messages
Total messages: 51 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Please take a look. I broke off the part that actually puts this all together into an add-on CL, but it's still pretty big. I can submit the different elements separately if that'd be easier, but I wanted to see how it fits together. It's a "rough draft" so I'd rather upload a bunch of small files than perfect any one component.
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #3 (id:140001) has been deleted
Cut CL # changed files in half!
khorimoto@chromium.org changed reviewers: + khorimoto@chromium.org
Are we going to use a selected page ID instead of index, as Jeremy mentioned on the design document? That would make things easier to understand IMO. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:5: <polymer-element name="cr-settings-main"> Shouldn't this contain the breadcrumbs at the top too? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:68: processPages_: function() { I don't understand why this is necessary. We declare the pages as children of the core-animated-pages anyway, so why do we need this extra processing? Also, why would we ever have a <template> here? We won't ever be using a "repeat" template here...is there another reason why this would be necessary? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:77: // Set the list of pages so each page is placed in a <section> within Why a <section>? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:135: /** Creates a section for 'core-animated-pages' for the given element. nit: Next line. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:141: element.setAttribute('cross-fade', ''); What's this doing? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.js:44: this.fire('cr-settings-page-select', Don't we need to declare this event type? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:11: on-cr-settings-page-select="{{pageSelected}}"> Should this be {{pageSelected_}}?
jlklein@chromium.org changed reviewers: + jlklein@chromium.org
https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. Following our current naming conventions, shouldn't this file just be drawer.css? c/b/r/settings/drawer/drawer.js c/b/r/settings/drawer/drawer.css c/b/r/settings/drawer/drawer.html I think the extra "settings" are repetitive. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:31: pages: [], Same comment as the other cl about initializing arrays and objects in created. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:44: * @type {Object} Worth a typedef here? Don't you also need an avatar? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:6: background-color: rgb(219, 222, 224); Same question as the other CL about where these color values came from. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:20: border-radius: 2px; I'm not sure I see where there's a border-radius for this in the mocks... https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:21: box-shadow: -2px 2px 3px 0 rgb(194, 197, 199); Shouldn't we be using a paper-shadow for this to match material shadows exactly? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:23: margin: 0 20px 0; RTL? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:71: var children = [].slice.call(this.$.pages.children); Why not just use this.$.pages.items? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:74: if (children[i].tagName != 'TEMPLATE') You can ignore this with an excludedLocalNames attribute on the core-animated-pages, but can you also explain why you need this? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:77: // Set the list of pages so each page is placed in a <section> within Why does it need to be inside a section? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:108: applyPageSplices_: function(splices) { I don't really understand why any of this stuff is necessary. My vision for keeping the pages attribute updates was basically the following: Initialize it to this.$.pages.items. Add a MutationObserver with childList:true on this.$.pages. In that callback, set this.pages to this.$.pages.items. I think this will be a much easier was to keep track of this stuff. The logic in here right now is pretty hard to follow and seems potentially fragile. Am I missing some reason all of this is needed? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.js:42: // Issue 33: selectedModel is not updated yet, so grab the model from the Weird... I guess it's either this or this.async(...). Does selectedModel get updated to the right thing (the page) after some amount of time? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.js:44: this.fire('cr-settings-page-select', On 2015/03/09 20:59:32, Kyle Horimoto wrote: > Don't we need to declare this event type? Yeah, we should probably at least extract this to a constant.
https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:23: margin: 0 20px 0; On 2015/03/09 21:17:14, Jeremy Klein wrote: > RTL? this is the same in LTR and RTL and could be shortened to: 0 20px https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:141: element.setAttribute('cross-fade', ''); On 2015/03/09 20:59:32, Kyle Horimoto wrote: > What's this doing? before: <element> after: <element cross-fade>
https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:23: margin: 0 20px 0; On 2015/03/09 21:23:30, Dan Beam wrote: > On 2015/03/09 21:17:14, Jeremy Klein wrote: > > RTL? > > this is the same in LTR and RTL and could be shortened to: 0 20px Ah yeah, are you overriding some left margin, though with 0?
https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. nit: Use "//", not "/* */". https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:141: element.setAttribute('cross-fade', ''); On 2015/03/09 21:23:30, Dan Beam wrote: > On 2015/03/09 20:59:32, Kyle Horimoto wrote: > > What's this doing? > > before: <element> > after: <element cross-fade> Well yeah - I know that, but I meant to ask why it was necessary and what it was accomplishing.
https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. On 2015/03/09 21:28:56, Kyle Horimoto wrote: > nit: Use "//", not "/* */". that syntax doesn't exist in CSS
orenb@chromium.org changed reviewers: + orenb@chromium.org
https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:25: * @type Array<!Obect> !Object https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:49: * @param {*} value The old value if the property value has changed, or the In the first case, what type would the old value have? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:59: this.updatePages_(this.pages); updatePages_ doesn't take args -- can you remove the arg from this call? https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:71: var children = [].slice.call(this.$.pages.children); How come we need to call slice in this unusual way (on an empty array and using 'call')?
https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. On 2015/03/09 21:33:14, Dan Beam wrote: > On 2015/03/09 21:28:56, Kyle Horimoto wrote: > > nit: Use "//", not "/* */". > > that syntax doesn't exist in CSS Oopsies! :)
Some answers to questions, will upload a new version soon. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:44: * @type {Object} On 2015/03/09 21:17:13, Jeremy Klein wrote: > Worth a typedef here? Don't you also need an avatar? Yeah. I'd rather keep this TODO for this CL. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:6: background-color: rgb(219, 222, 224); On 2015/03/09 21:17:14, Jeremy Klein wrote: > Same question as the other CL about where these color values came from. Straight from the mocks. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:20: border-radius: 2px; On 2015/03/09 21:17:14, Jeremy Klein wrote: > I'm not sure I see where there's a border-radius for this in the mocks... You're right, will remove. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:5: <polymer-element name="cr-settings-main"> On 2015/03/09 20:59:32, Kyle Horimoto wrote: > Shouldn't this contain the breadcrumbs at the top too? At some point :-) We should nail down the design for pages/subpages before this. Also the search field will be a pain. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:68: processPages_: function() { On 2015/03/09 20:59:32, Kyle Horimoto wrote: > I don't understand why this is necessary. We declare the pages as children of > the core-animated-pages anyway, so why do we need this extra processing? Some annoying rule about containers for pages not participating in the transition. https://www.polymer-project.org/docs/elements/core-elements.html#core-animate... Search for "this does not work". >Also, > why would we ever have a <template> here? We won't ever be using a "repeat" > template here...is there another reason why this would be necessary? Oops, in a prior version I had <template if="{{isChromeOs()}}"> surrounding a11y-page, but there are no <template>s here. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:71: var children = [].slice.call(this.$.pages.children); On 2015/03/09 21:34:00, Oren Blasberg wrote: > How come we need to call slice in this unusual way (on an empty array and using > 'call')? Oops, this was from when I was updating the pages in place (children returns a live HTMLCollection so this would convert it to a static array). (Using the empty array is a sort of shorthand for Array.prototype.) https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:74: if (children[i].tagName != 'TEMPLATE') On 2015/03/09 21:17:14, Jeremy Klein wrote: > You can ignore this with an excludedLocalNames attribute on the > core-animated-pages, but can you also explain why you need this? See response to khorimito above. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:77: // Set the list of pages so each page is placed in a <section> within On 2015/03/09 21:17:14, Jeremy Klein wrote: > Why does it need to be inside a section? Requirement of core-animated-pages, provides a container so the page can be animated in/out. See response to khorimoto above. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:108: applyPageSplices_: function(splices) { On 2015/03/09 21:17:14, Jeremy Klein wrote: > I don't really understand why any of this stuff is necessary. My vision for > keeping the pages attribute updates was basically the following: > > Initialize it to this.$.pages.items. > Add a MutationObserver with childList:true on this.$.pages. > In that callback, set this.pages to this.$.pages.items. > > I think this will be a much easier was to keep track of this stuff. The logic in > here right now is pretty hard to follow and seems potentially fragile. Am I > missing some reason all of this is needed? Hmm, I'll take a look at MutationObserver. I agree this is fragile and I don't like it. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:141: element.setAttribute('cross-fade', ''); On 2015/03/09 21:28:56, Kyle Horimoto wrote: > On 2015/03/09 21:23:30, Dan Beam wrote: > > On 2015/03/09 20:59:32, Kyle Horimoto wrote: > > > What's this doing? > > > > before: <element> > > after: <element cross-fade> > > Well yeah - I know that, but I meant to ask why it was necessary and what it was > accomplishing. It tells core-animated-pages that this page should be included in the cross-fade transition. We can set this from HTML though.
https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:68: processPages_: function() { On 2015/03/09 22:24:11, michaelpg wrote: > On 2015/03/09 20:59:32, Kyle Horimoto wrote: > > I don't understand why this is necessary. We declare the pages as children of > > the core-animated-pages anyway, so why do we need this extra processing? > > Some annoying rule about containers for pages not participating in the > transition. > https://www.polymer-project.org/docs/elements/core-elements.html#core-animate... > Search for "this does not work". > > >Also, > > why would we ever have a <template> here? We won't ever be using a "repeat" > > template here...is there another reason why this would be necessary? > > Oops, in a prior version I had <template if="{{isChromeOs()}}"> surrounding > a11y-page, but there are no <template>s here. Just use the "cross-fade-all" transition instead of "cross-fade", and I think all this is unnecessary.
https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:77: // Set the list of pages so each page is placed in a <section> within On 2015/03/09 22:24:11, michaelpg wrote: > On 2015/03/09 21:17:14, Jeremy Klein wrote: > > Why does it need to be inside a section? > > Requirement of core-animated-pages, provides a container so the page can be > animated in/out. See response to khorimoto above. Section isn't a requirement of core-animated-pages. There's just a restriction that the container itself can't animate. Did you try out cross-fade-all on the core-animated-pages for now? The only practical implication of this restriction is that we won't put transitions on the page element itself, but rather on individual elements within it. I feel like we should just use cross-fade-all for now and once we have more specs for specific transitions, we can transition whatever we want inside the pages. I just don't really like programmatically wrapping the pages in sections because then the sections are the core-animation-page's items rather than the pages and selection binding becomes a bit more convoluted. I think we can avoid this. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:141: element.setAttribute('cross-fade', ''); On 2015/03/09 22:24:11, michaelpg wrote: > On 2015/03/09 21:28:56, Kyle Horimoto wrote: > > On 2015/03/09 21:23:30, Dan Beam wrote: > > > On 2015/03/09 20:59:32, Kyle Horimoto wrote: > > > > What's this doing? > > > > > > before: <element> > > > after: <element cross-fade> > > > > Well yeah - I know that, but I meant to ask why it was necessary and what it > was > > accomplishing. > > It tells core-animated-pages that this page should be included in the cross-fade > transition. We can set this from HTML though. See note above about cross-fade-all.
https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.css:9: overflow: auto; one space after : https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.html (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.html:7: <div id="user-card" class="medium" horizontal layout center> nit: 'layout vertical' above, 'horizontal layout' here. (I know it doesn't matter, just a little confusing to read). Also, does it make sense to make the user card its own component? I don't know if we'll ever use it elsewhere, but it does feel rather componenty. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:6: background-color: rgb(219, 222, 224); On 2015/03/09 22:24:11, michaelpg wrote: > On 2015/03/09 21:17:14, Jeremy Klein wrote: > > Same question as the other CL about where these color values came from. > > Straight from the mocks. Weren't we going to look into a better way to define colors? Not that I have any suggestions mind you.
https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:6: background-color: rgb(219, 222, 224); On 2015/03/10 17:22:53, stevenjb wrote: > On 2015/03/09 22:24:11, michaelpg wrote: > > On 2015/03/09 21:17:14, Jeremy Klein wrote: > > > Same question as the other CL about where these color values came from. > > > > Straight from the mocks. > > Weren't we going to look into a better way to define colors? Not that I have any > suggestions mind you. Michael and I talked about this briefly off-thread and we think this can probably be done using core-style, but it's probably worth doing in another CL. For now, I think we should go with colors from: http://www.google.com/design/spec/style/color.html
https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:6: background-color: rgb(219, 222, 224); On 2015/03/10 17:27:05, Jeremy Klein wrote: > On 2015/03/10 17:22:53, stevenjb wrote: > > On 2015/03/09 22:24:11, michaelpg wrote: > > > On 2015/03/09 21:17:14, Jeremy Klein wrote: > > > > Same question as the other CL about where these color values came from. > > > > > > Straight from the mocks. > > > > Weren't we going to look into a better way to define colors? Not that I have > any > > suggestions mind you. > > Michael and I talked about this briefly off-thread and we think this can > probably be done using core-style, but it's probably worth doing in another CL. > For now, I think we should go with colors from: > > http://www.google.com/design/spec/style/color.html sgtm
Patchset #6 (id:190018) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
Patchset #4 (id:230001) has been deleted
Simplified. PTAL. Thanks! https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.css:9: overflow: auto; On 2015/03/10 17:22:53, stevenjb wrote: > one space after : Done. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.html (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.html:7: <div id="user-card" class="medium" horizontal layout center> On 2015/03/10 17:22:53, stevenjb wrote: > nit: 'layout vertical' above, 'horizontal layout' here. (I know it doesn't > matter, just a little confusing to read). Done. > > Also, does it make sense to make the user card its own component? I don't know > if we'll ever use it elsewhere, but it does feel rather componenty. Yes. I think it would add more noise to this CL though, so I've added a TODO. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:31: pages: [], On 2015/03/09 21:17:13, Jeremy Klein wrote: > Same comment as the other cl about initializing arrays and objects in created. Missed that comment, thank you. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:6: background-color: rgb(219, 222, 224); On 2015/03/10 17:27:05, Jeremy Klein wrote: > On 2015/03/10 17:22:53, stevenjb wrote: > > On 2015/03/09 22:24:11, michaelpg wrote: > > > On 2015/03/09 21:17:14, Jeremy Klein wrote: > > > > Same question as the other CL about where these color values came from. > > > > > > Straight from the mocks. > > > > Weren't we going to look into a better way to define colors? Not that I have > any > > suggestions mind you. > > Michael and I talked about this briefly off-thread and we think this can > probably be done using core-style, but it's probably worth doing in another CL. > For now, I think we should go with colors from: > > http://www.google.com/design/spec/style/color.html I spoke with Cody, who said he's still waiting to hear back about the colors. But he did say the deviation from the specs was intentional. Rather than do the work of finding colors from the spec that could change soon, I think we should just move forward with the understanding that colors (like the rest of the UI) haven't been signed off on. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:21: box-shadow: -2px 2px 3px 0 rgb(194, 197, 199); On 2015/03/09 21:17:14, Jeremy Klein wrote: > Shouldn't we be using a paper-shadow for this to match material shadows exactly? Done. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:23: margin: 0 20px 0; On 2015/03/09 21:25:18, Jeremy Klein wrote: > On 2015/03/09 21:23:30, Dan Beam wrote: > > On 2015/03/09 21:17:14, Jeremy Klein wrote: > > > RTL? > > > > this is the same in LTR and RTL and could be shortened to: 0 20px > > Ah yeah, are you overriding some left margin, though with 0? I'm adding a 20px margin on either side. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:25: * @type Array<!Obect> On 2015/03/09 21:34:00, Oren Blasberg wrote: > !Object Done. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:49: * @param {*} value The old value if the property value has changed, or the On 2015/03/09 21:34:00, Oren Blasberg wrote: > In the first case, what type would the old value have? Removed. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:59: this.updatePages_(this.pages); On 2015/03/09 21:34:00, Oren Blasberg wrote: > updatePages_ doesn't take args -- can you remove the arg from this call? Removed. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:68: processPages_: function() { On 2015/03/09 22:32:03, Kyle Horimoto wrote: > On 2015/03/09 22:24:11, michaelpg wrote: > > On 2015/03/09 20:59:32, Kyle Horimoto wrote: > > > I don't understand why this is necessary. We declare the pages as children > of > > > the core-animated-pages anyway, so why do we need this extra processing? > > > > Some annoying rule about containers for pages not participating in the > > transition. > > > https://www.polymer-project.org/docs/elements/core-elements.html#core-animate... > > Search for "this does not work". > > > > >Also, > > > why would we ever have a <template> here? We won't ever be using a "repeat" > > > template here...is there another reason why this would be necessary? > > > > Oops, in a prior version I had <template if="{{isChromeOs()}}"> surrounding > > a11y-page, but there are no <template>s here. > > Just use the "cross-fade-all" transition instead of "cross-fade", and I think > all this is unnecessary. Done-ish (cross-fade-all is too janky) https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:77: // Set the list of pages so each page is placed in a <section> within On 2015/03/09 22:35:05, Jeremy Klein wrote: > On 2015/03/09 22:24:11, michaelpg wrote: > > On 2015/03/09 21:17:14, Jeremy Klein wrote: > > > Why does it need to be inside a section? > > > > Requirement of core-animated-pages, provides a container so the page can be > > animated in/out. See response to khorimoto above. > > Section isn't a requirement of core-animated-pages. There's just a restriction > that the container itself can't animate. Did you try out cross-fade-all on the > core-animated-pages for now? The only practical implication of this restriction > is that we won't put transitions on the page element itself, but rather on > individual elements within it. I feel like we should just use cross-fade-all for > now and once we have more specs for specific transitions, we can transition > whatever we want inside the pages. > > I just don't really like programmatically wrapping the pages in sections because > then the sections are the core-animation-page's items rather than the pages and > selection binding becomes a bit more convoluted. I think we can avoid this. Done. Thanks for the tips Jeremy & Kyle! https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:108: applyPageSplices_: function(splices) { On 2015/03/09 22:24:11, michaelpg wrote: > On 2015/03/09 21:17:14, Jeremy Klein wrote: > > I don't really understand why any of this stuff is necessary. My vision for > > keeping the pages attribute updates was basically the following: > > > > Initialize it to this.$.pages.items. > > Add a MutationObserver with childList:true on this.$.pages. > > In that callback, set this.pages to this.$.pages.items. > > > > I think this will be a much easier was to keep track of this stuff. The logic > in > > here right now is pretty hard to follow and seems potentially fragile. Am I > > missing some reason all of this is needed? > > Hmm, I'll take a look at MutationObserver. I agree this is fragile and I don't > like it. Done. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:141: element.setAttribute('cross-fade', ''); On 2015/03/09 22:35:05, Jeremy Klein wrote: > On 2015/03/09 22:24:11, michaelpg wrote: > > On 2015/03/09 21:28:56, Kyle Horimoto wrote: > > > On 2015/03/09 21:23:30, Dan Beam wrote: > > > > On 2015/03/09 20:59:32, Kyle Horimoto wrote: > > > > > What's this doing? > > > > > > > > before: <element> > > > > after: <element cross-fade> > > > > > > Well yeah - I know that, but I meant to ask why it was necessary and what it > > was > > > accomplishing. > > > > It tells core-animated-pages that this page should be included in the > cross-fade > > transition. We can set this from HTML though. > > See note above about cross-fade-all. Done. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.js:44: this.fire('cr-settings-page-select', On 2015/03/09 20:59:32, Kyle Horimoto wrote: > Don't we need to declare this event type? Removed, as this isn't doing anything. https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:11: on-cr-settings-page-select="{{pageSelected}}"> On 2015/03/09 20:59:32, Kyle Horimoto wrote: > Should this be {{pageSelected_}}? Removed.
This is much better, Michael. Awesome work and thank you for your patience on this! https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.css (right): https://codereview.chromium.org/981203007/diff/160001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:6: background-color: rgb(219, 222, 224); On 2015/03/12 01:41:39, michaelpg wrote: > On 2015/03/10 17:27:05, Jeremy Klein wrote: > > On 2015/03/10 17:22:53, stevenjb wrote: > > > On 2015/03/09 22:24:11, michaelpg wrote: > > > > On 2015/03/09 21:17:14, Jeremy Klein wrote: > > > > > Same question as the other CL about where these color values came from. > > > > > > > > Straight from the mocks. > > > > > > Weren't we going to look into a better way to define colors? Not that I have > > any > > > suggestions mind you. > > > > Michael and I talked about this briefly off-thread and we think this can > > probably be done using core-style, but it's probably worth doing in another > CL. > > For now, I think we should go with colors from: > > > > http://www.google.com/design/spec/style/color.html > > I spoke with Cody, who said he's still waiting to hear back about the colors. > But he did say the deviation from the specs was intentional. Rather than do the > work of finding colors from the spec that could change soon, I think we should > just move forward with the understanding that colors (like the rest of the UI) > haven't been signed off on. Ok, sounds good to me. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:55: email: 'user@example.com', nit: Can you also add a fake avatar url just so we don't forget about it when we make a real object. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:9: <paper-shadow flex> Do we want flex or fit here? The container doesn't seem to be display: flex; so I'm not sure how this affects anything. We might just be able to get rid of it. I assume it's here for when the help articles come in, right? Maybe we should just wait until that lands to add the flexing here and in the css. Also, when you do that, you can write "flex two" and get rid of the flex: 2 in the css. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:11: valueattr="PAGE_ID" excludeLocalNames="template" nit: not sure you need the excludedLocalNames any more. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:7: * 'cr-settings-main' displays the selected settings page. Did we decide to go with back ticks here? I don't mind either way as long as we're consistent. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:14: * See cr-settings-drawer for example of use in 'core-panel'. s/core-panel/core-drawer-panel. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:56: var observer = new MutationObserver(this.pagesUpdated_.bind(this)); This stuff is great and much more readable IMO. Thanks! https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.html:9: <paper-item id="{{PAGE_ID}}"> nit: Is there any reason we need to use the id attribute here? I'd rather just call it pageId or something to avoid confusion. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.html:11: {{pageTitle}} Shouldn't we add a pageTitle and icon to the a11y page? https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:30: * To avoid unnecessary DOM manipulation, prefer using Array.prototype Is this comment still applicable if we add the pages declaratively? Maybe even just saying "Don't edit this variable directly" is enough.
https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.js (right): https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.js:55: email: 'user@example.com', On 2015/03/12 02:11:18, Jeremy Klein wrote: > nit: Can you also add a fake avatar url just so we don't forget about it when we > make a real object. Done. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:9: <paper-shadow flex> On 2015/03/12 02:11:18, Jeremy Klein wrote: > Do we want flex or fit here? The container doesn't seem to be display: flex; so > I'm not sure how this affects anything. We might just be able to get rid of it. > I assume it's here for when the help articles come in, right? Maybe we should > just wait until that lands to add the flexing here and in the css. Also, when > you do that, you can write "flex two" and get rid of the flex: 2 in the css. Spot on. Removed for now, thanks. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:11: valueattr="PAGE_ID" excludeLocalNames="template" On 2015/03/12 02:11:18, Jeremy Klein wrote: > nit: not sure you need the excludedLocalNames any more. Done. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:7: * 'cr-settings-main' displays the selected settings page. On 2015/03/12 02:11:18, Jeremy Klein wrote: > Did we decide to go with back ticks here? I don't mind either way as long as > we're consistent. I think we're mostly not using them now. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:14: * See cr-settings-drawer for example of use in 'core-panel'. On 2015/03/12 02:11:18, Jeremy Klein wrote: > s/core-panel/core-drawer-panel. Done. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.html:9: <paper-item id="{{PAGE_ID}}"> On 2015/03/12 02:11:18, Jeremy Klein wrote: > nit: Is there any reason we need to use the id attribute here? I'd rather just > call it pageId or something to avoid confusion. Done. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.html:11: {{pageTitle}} On 2015/03/12 02:11:18, Jeremy Klein wrote: > Shouldn't we add a pageTitle and icon to the a11y page? yes, those should have been there -- i'm doing a bad job maintaining my branches :-( https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:30: * To avoid unnecessary DOM manipulation, prefer using Array.prototype On 2015/03/12 02:11:18, Jeremy Klein wrote: > Is this comment still applicable if we add the pages declaratively? Maybe even > just saying "Don't edit this variable directly" is enough. Done.
lgtm Only nits and simple questions left. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:7: * 'cr-settings-main' displays the selected settings page. On 2015/03/12 06:29:37, michaelpg wrote: > On 2015/03/12 02:11:18, Jeremy Klein wrote: > > Did we decide to go with back ticks here? I don't mind either way as long as > > we're consistent. > > I think we're mostly not using them now. Sounds good to me. Let's stick to that, then. https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/981203007/diff/250001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.html:11: {{pageTitle}} On 2015/03/12 06:29:37, michaelpg wrote: > On 2015/03/12 02:11:18, Jeremy Klein wrote: > > Shouldn't we add a pageTitle and icon to the a11y page? > > yes, those should have been there -- i'm doing a bad job maintaining my branches > :-( No worries, it's hard to untangle lots of commits :-). Just today I accidentally put a file in the wrong CL twice haha. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.css (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:20: flex: 2; Let's get rid of this in this CL too. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:24: core-animated-pages > section { Should this just be core-animated-pages > * now? https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:12: valueattr="PAGE_ID" transitions="cross-fade"> For the cross-fade, are you just thinking we'll add the cross fade on elements inside the pages where appropriate? https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:8: <core-drawer-panel drawerWidth="280px"> Just curious: How does the responsive design look with a really small window? Is there a way to open and close the drawer easily?
https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:11: <core-animated-pages id="pages" selected="{{selectedPageId}}" nit: Do you think it would be better to give this an ID of "page-selector", or something a little different from the "pages" field used by most of the custom elements you create in this CL? https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:68: */ nit: @private https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:82: this.pages = this.$.pages.items; This assignment is bound outward all the way up to settings-ui, right? Just want to make sure I'm understanding this correctly.
This looks great Michael. I am really impressed at how clean the implementation has turned out. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.js (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:12: * <core-animated-pages> Can you make the same changes to downloads_page as well? https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:13: * <cr-settings-a11y-page prefs="{{prefs}}"></cr-settings-a11y-page> This was probably answered in the 60+ earlier comments :/ so I apologize in advance, but what motivated this particular change (to have prefs specified declaratively)? Was it spurred by something going wrong with the previous approach or was it because this just is a nicer way to do things? https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:36: * default 'a11y' nit: @default https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.html (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.html:15: <cr-settings-menu flex selectedId="{{selectedId}}" pages="{{pages}}"> Side note: As I am learning polymer I'm starting to expect certain 'magic' things and am a bit surprised that new attributes like 'selectedId' aren't automatically converted from camelCase in the JS, to dashed-notation in the html :P I wonder if that's in the pipe for them. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:70: if (!this.pages.length) optional nit: from years of JS now I prefer using curly braces even when the body is 1 line because I frequently forget to add them later and add bugs on accident. Up to you though https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.html:11: {{pageTitle}} I must've missed it someplace, but where does pageTitle get declared/defined? https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:8: <core-drawer-panel drawerWidth="280px"> Could the drawerWidth=280 be styled via css? Could we put it in the settings_ui.css by piercing the cr-settings-ui's shadow dom with ::shadow and grabbing the core-drawer-panel on it. (again, if this is a Q that was already answered, I am sorry)
Thanks for the reviews. PTAL, and remember we'll continue iterating on this design! https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/a11y_page/a11y_page.js (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:12: * <core-animated-pages> On 2015/03/12 18:58:17, Oren Blasberg wrote: > Can you make the same changes to downloads_page as well? Done. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:13: * <cr-settings-a11y-page prefs="{{prefs}}"></cr-settings-a11y-page> On 2015/03/12 18:58:17, Oren Blasberg wrote: > This was probably answered in the 60+ earlier comments :/ so I apologize in > advance, but what motivated this particular change (to have prefs specified > declaratively)? Was it spurred by something going wrong with the previous > approach or was it because this just is a nicer way to do things? It wasn't done declaratively in a11y_page/demo.html because we aren't using templates there. It's easier this way IMO. I'm just adding it to the example to indicate that the attribute should be set somehow. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/a11y_page/a11y_page.js:36: * default 'a11y' On 2015/03/12 18:58:17, Oren Blasberg wrote: > nit: @default Done. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_drawer/settings_drawer.html (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_drawer/settings_drawer.html:15: <cr-settings-menu flex selectedId="{{selectedId}}" pages="{{pages}}"> On 2015/03/12 18:58:17, Oren Blasberg wrote: > Side note: As I am learning polymer I'm starting to expect certain 'magic' > things and am a bit surprised that new attributes like 'selectedId' aren't > automatically converted from camelCase in the JS, to dashed-notation in the html > :P I wonder if that's in the pipe for them. Yeah! Attribute conversion to camelCase already exists in 0.8. There's nothing for this.$ yet so I filed: https://github.com/Polymer/polymer/issues/1290 https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.css (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:20: flex: 2; On 2015/03/12 06:46:36, Jeremy Klein wrote: > Let's get rid of this in this CL too. Done. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.css:24: core-animated-pages > section { On 2015/03/12 06:46:36, Jeremy Klein wrote: > Should this just be core-animated-pages > * now? Removed. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:11: <core-animated-pages id="pages" selected="{{selectedPageId}}" On 2015/03/12 18:57:35, Kyle Horimoto wrote: > nit: Do you think it would be better to give this an ID of "page-selector", or > something a little different from the "pages" field used by most of the custom > elements you create in this CL? Good idea. I used 'pageContainer'. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:12: valueattr="PAGE_ID" transitions="cross-fade"> On 2015/03/12 06:46:36, Jeremy Klein wrote: > For the cross-fade, are you just thinking we'll add the cross fade on elements > inside the pages where appropriate? No, we'll wrap the entire page in a <section cross-fade>. Faster and easier than cross-fading a bunch of elements. Check out the changes to a11y_page.html in the next CL here: https://codereview.chromium.org/986843002/diff2/100001:120001/chrome/browser/... I figure it's not worth modifying a11y_page.html for this CL, especially since there's only one page. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:68: */ On 2015/03/12 18:57:35, Kyle Horimoto wrote: > nit: @private Done. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:70: if (!this.pages.length) On 2015/03/12 18:58:17, Oren Blasberg wrote: > optional nit: from years of JS now I prefer using curly braces even when the > body is 1 line because I frequently forget to add them later and add bugs on > accident. Up to you though Thanks, but I'm going to respectfully ignore this. Optional curly braces is one of the few explicit Chromium JS rules, and most of the JS I read and write omits the braces. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:82: this.pages = this.$.pages.items; On 2015/03/12 18:57:35, Kyle Horimoto wrote: > This assignment is bound outward all the way up to settings-ui, right? Just want > to make sure I'm understanding this correctly. Yep, and then back down to settings-drawer -> settings-menu. Kind of crazy I guess, something to consider when we work on a navigation model. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.html:11: {{pageTitle}} On 2015/03/12 18:58:17, Oren Blasberg wrote: > I must've missed it someplace, but where does pageTitle get declared/defined? When binding a template to an object, all references in {{}} are scoped to the object. If I had used 'repeat="{{page in pages}}"' then the reference would be {{page.pageTitle}}. Some good examples at https://www.polymer-project.org/0.5/docs/polymer/template.html So pageTitle is defined in a11y_page.js. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:8: <core-drawer-panel drawerWidth="280px"> On 2015/03/12 18:58:17, Oren Blasberg wrote: > Could the drawerWidth=280 be styled via css? Could we put it in the > settings_ui.css by piercing the cr-settings-ui's shadow dom with ::shadow and > grabbing the core-drawer-panel on it. > (again, if this is a Q that was already answered, I am sorry) Not that I can tell, it seems to be an attribute that the JS of core-drawer-panel uses. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:8: <core-drawer-panel drawerWidth="280px"> On 2015/03/12 06:46:36, Jeremy Klein wrote: > Just curious: How does the responsive design look with a really small window? Is > there a way to open and close the drawer easily? It's kind of silly and may not be the best approach, but I don't want to play around with the specifics until the UI is approved. I added a core-icon-button to open the drawer when the window is small (clicking outside the drawer closes it).
lgtm https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:12: valueattr="PAGE_ID" transitions="cross-fade"> On 2015/03/13 00:47:01, michaelpg wrote: > On 2015/03/12 06:46:36, Jeremy Klein wrote: > > For the cross-fade, are you just thinking we'll add the cross fade on elements > > inside the pages where appropriate? > > No, we'll wrap the entire page in a <section cross-fade>. Faster and easier than > cross-fading a bunch of elements. > > Check out the changes to a11y_page.html in the next CL here: > https://codereview.chromium.org/986843002/diff2/100001:120001/chrome/browser/... > > I figure it's not worth modifying a11y_page.html for this CL, especially since > there's only one page. Cool, that works too. Thanks. https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/981203007/diff/270001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:8: <core-drawer-panel drawerWidth="280px"> On 2015/03/13 00:47:01, michaelpg wrote: > On 2015/03/12 06:46:36, Jeremy Klein wrote: > > Just curious: How does the responsive design look with a really small window? > Is > > there a way to open and close the drawer easily? > > It's kind of silly and may not be the best approach, but I don't want to play > around with the specifics until the UI is approved. I added a core-icon-button > to open the drawer when the window is small (clicking outside the drawer closes > it). Yeah, that's fine with me.
lgtm
Thanks all. Technically I only needed the L-G-T-M from khorimoto@ but stevenjb@ and jhawkins@, LMK whether you want me to wait on your L-G-T-Ms. I'd be happy to get more feedback if you want to take another look, otherwise I'll submit this. Michael
Go ahead and we'll iterate on this foundation.
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jlklein@chromium.org, khorimoto@chromium.org Link to the patchset: https://codereview.chromium.org/981203007/#ps310001 (title: "icon fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981203007/310001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jlklein@chromium.org, khorimoto@chromium.org, orenb@chromium.org Link to the patchset: https://codereview.chromium.org/981203007/#ps330001 (title: "license")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981203007/330001
Message was sent while issue was closed.
Committed patchset #8 (id:330001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8be27dccec415091458637d80a46151a0ba3038f Cr-Commit-Position: refs/heads/master@{#320436} |
