Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2008843003:
[MD settings] redesign of side nav (Closed)
|
Created:
4 years, 7 months ago by dschuyler Modified:
4 years, 6 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] redesign of side nav
This CL reworks the side nav menu to remove the 'basic' menu label while
keeping the items in the basic menu displayed. The 'advanced' page is
now toggled separately from the basic page. An 'Advanced' toggle button
has also been added to the content area which will expand or collapse
the advanced settings.
BUG=593989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/70ae59c616a8f0b948c143a29027cbf7a8872851
Cr-Commit-Position: refs/heads/master@{#398434}
Patch Set 1 : cleanup #Patch Set 2 : fix for open advanced #
Total comments: 10
Patch Set 3 : fix unit tests #
Total comments: 14
Patch Set 4 : review changes #
Total comments: 10
Patch Set 5 : change defaults #
Total comments: 1
Messages
Total messages: 33 (16 generated)
Description was changed from ========== [MD settings] redesign of side nav This CL reworks the side nav menu to remove the 'basic' menu label while keeping the items in the basic menu displayed. The 'advanced' page is now toggled separately from the basic page. An 'Advanced' toggle button has also been added to the content area which will expand or collapse the advanced settings. BUG=593989 ========== to ========== [MD settings] redesign of side nav This CL reworks the side nav menu to remove the 'basic' menu label while keeping the items in the basic menu displayed. The 'advanced' page is now toggled separately from the basic page. An 'Advanced' toggle button has also been added to the content area which will expand or collapse the advanced settings. BUG=593989 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
dschuyler@chromium.org changed reviewers: + dbeam@chromium.org
On 2016/05/25 23:01:52, dschuyler wrote: I found a problem with subpages on this CL. I'm looking into it. Feel free to hold off on the review until Patch #2.
ping this when you actually need a review (commenting so it's out of my queue)
Patchset #2 (id:100001) has been deleted
On 2016/05/31 21:29:17, Dan Beam wrote: > ping this when you actually need a review (commenting so it's out of my queue) PTAL
https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:11: @apply(--layout-center); i don't really see the need for @apply(--layout*) stuff, it just adds cross-browser goop additionally, you'd probably want to do @apply(--layout); @apply(--layout-center); @apply(--layout-center-justified); if you really want to be polymeric, but again: same problem (just more goop) i vote we remove all these in the code we control because chrome-only code doesn't need it https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:14: padding: 0 12px; /** TODO(dschuyler): get real value. */ /** -> /* https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:6: is: 'advanced-page-toggle', can this be implemented in simple DOM instead of custom element + shadow DOM? https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:89: this.showBasicPage_ = !(newRoute.page == 'about' || can you use this.showAboutPage_ and this.showAdvancedToggle_ to implement this.showBasicPage_? https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.html:77: hidden$="!basicOpened_"> where is basicOpened_ defined? also, why would we ever hide these?
Patchset #3 (id:140001) has been deleted
https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:11: @apply(--layout-center); On 2016/05/31 23:28:24, Dan Beam wrote: > i don't really see the need for @apply(--layout*) stuff, it just adds > cross-browser goop > > additionally, you'd probably want to do > > @apply(--layout); > @apply(--layout-center); > @apply(--layout-center-justified); > > if you really want to be polymeric, but again: same problem (just more goop) > > i vote we remove all these in the code we control because chrome-only code > doesn't need it Done. https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:14: padding: 0 12px; /** TODO(dschuyler): get real value. */ On 2016/05/31 23:28:24, Dan Beam wrote: > /** -> /* Done. https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:6: is: 'advanced-page-toggle', On 2016/05/31 23:28:24, Dan Beam wrote: > can this be implemented in simple DOM instead of custom element + shadow DOM? Done. https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:89: this.showBasicPage_ = !(newRoute.page == 'about' || On 2016/05/31 23:28:24, Dan Beam wrote: > can you use this.showAboutPage_ and this.showAdvancedToggle_ to implement > this.showBasicPage_? Done. https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.html (right): https://codereview.chromium.org/2008843003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.html:77: hidden$="!basicOpened_"> On 2016/05/31 23:28:24, Dan Beam wrote: > where is basicOpened_ defined? also, why would we ever hide these? Done.
https://codereview.chromium.org/2008843003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2008843003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:12: } "core-" are from pre-1.0 polymer, so this is dead code. remove or fix https://codereview.chromium.org/2008843003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2008843003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:52: this.isAdvancedMenuOpen_ = e.detail; does this compile? do you need to cast |e| to a CustomEvent? https://codereview.chromium.org/2008843003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:70: this.showAdvancedToggle_ || newRoute.page == 'advanced'; add extra parens cuz ambiguity https://codereview.chromium.org/2008843003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:72: this.$.pageContainer.style.height = (isSubpage ? '100%' : ''); drop extra parens https://codereview.chromium.org/2008843003/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_menu_test.js (right): https://codereview.chromium.org/2008843003/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_menu_test.js:25: return Promise.resolve().then(function() { what is this Promise.resolve() voodoo? https://codereview.chromium.org/2008843003/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_menu_test.js:31: return; wat
try to use callback (i.e. events or public-er promises) rather than async() or Promise.resolve() https://codereview.chromium.org/2008843003/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_menu_test.js (right): https://codereview.chromium.org/2008843003/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_menu_test.js:43: }).then(function() { indent off
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
Patchset #4 (id:220001) has been deleted
Patchset #4 (id:240001) has been deleted
https://codereview.chromium.org/2008843003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2008843003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.html:12: } On 2016/06/02 01:28:19, Dan Beam wrote: > "core-" are from pre-1.0 polymer, so this is dead code. remove or fix Done. https://codereview.chromium.org/2008843003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2008843003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:52: this.isAdvancedMenuOpen_ = e.detail; On 2016/06/02 01:28:19, Dan Beam wrote: > does this compile? do you need to cast |e| to a CustomEvent? I added a compiled_resources2.gyp to test this. e.detail is accepted as a bool without a cast. https://codereview.chromium.org/2008843003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:70: this.showAdvancedToggle_ || newRoute.page == 'advanced'; On 2016/06/02 01:28:20, Dan Beam wrote: > add extra parens cuz ambiguity Done. https://codereview.chromium.org/2008843003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:72: this.$.pageContainer.style.height = (isSubpage ? '100%' : ''); On 2016/06/02 01:28:19, Dan Beam wrote: > drop extra parens Done. https://codereview.chromium.org/2008843003/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_menu_test.js (right): https://codereview.chromium.org/2008843003/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_menu_test.js:25: return Promise.resolve().then(function() { On 2016/06/02 01:28:20, Dan Beam wrote: > what is this Promise.resolve() voodoo? Done. https://codereview.chromium.org/2008843003/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_menu_test.js:31: return; On 2016/06/02 01:28:20, Dan Beam wrote: > wat Done. https://codereview.chromium.org/2008843003/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_menu_test.js:43: }).then(function() { On 2016/06/02 01:39:57, Dan Beam wrote: > indent off Done.
this looks much better https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:58: currentRouteChanged_: function(newRoute) { does this get called immediately after load or something? how do the initial values get set correctly? https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.js:44: this.$.advancedPage.open(); could this be: this.fire('toggle-advanced-page', this.currentRoute.page == 'advanced');
https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:58: currentRouteChanged_: function(newRoute) { On 2016/06/03 03:12:24, Dan Beam wrote: > does this get called immediately after load or something? I believe so. I don't know of a case where it doesn't. > how do the initial > values get set correctly? Via this function (or the polymer defaults). https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.js:44: this.$.advancedPage.open(); On 2016/06/03 03:12:24, Dan Beam wrote: > could this be: > > this.fire('toggle-advanced-page', this.currentRoute.page == 'advanced'); Yes, but then it would fire an unnecessary event during initial page load in the common case (going to the basic settings).
lgtm https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:38: showAdvancedToggle_: Boolean, showAdvancedToggle_: { type: Boolean, value: undefined // maybe? or true? }, https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:41: showBasicPage_: Boolean, make true by default? or undefined? https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.js:44: this.$.advancedPage.open(); On 2016/06/06 21:47:55, dschuyler wrote: > On 2016/06/03 03:12:24, Dan Beam wrote: > > could this be: > > > > this.fire('toggle-advanced-page', this.currentRoute.page == 'advanced'); > > Yes, but then it would fire an unnecessary event > during initial page load in the common case (going > to the basic settings). it seems like this would no-op on the other side and make your code slightly simpler here
https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:38: showAdvancedToggle_: Boolean, On 2016/06/06 22:31:03, Dan Beam wrote: > showAdvancedToggle_: { > type: Boolean, > value: undefined // maybe? or true? > }, Done. https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:41: showBasicPage_: Boolean, On 2016/06/06 22:31:03, Dan Beam wrote: > make true by default? or undefined? Done. https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2008843003/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/settings_menu/settings_menu.js:44: this.$.advancedPage.open(); On 2016/06/06 22:31:03, Dan Beam wrote: > On 2016/06/06 21:47:55, dschuyler wrote: > > On 2016/06/03 03:12:24, Dan Beam wrote: > > > could this be: > > > > > > this.fire('toggle-advanced-page', this.currentRoute.page == 'advanced'); > > > > Yes, but then it would fire an unnecessary event > > during initial page load in the common case (going > > to the basic settings). > > it seems like this would no-op on the other side and make your code slightly > simpler here Done.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2008843003/#ps280001 (title: "change defaults")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008843003/280001
Message was sent while issue was closed.
Committed patchset #5 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [MD settings] redesign of side nav This CL reworks the side nav menu to remove the 'basic' menu label while keeping the items in the basic menu displayed. The 'advanced' page is now toggled separately from the basic page. An 'Advanced' toggle button has also been added to the content area which will expand or collapse the advanced settings. BUG=593989 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] redesign of side nav This CL reworks the side nav menu to remove the 'basic' menu label while keeping the items in the basic menu displayed. The 'advanced' page is now toggled separately from the basic page. An 'Advanced' toggle button has also been added to the content area which will expand or collapse the advanced settings. BUG=593989 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/70ae59c616a8f0b948c143a29027cbf7a8872851 Cr-Commit-Position: refs/heads/master@{#398434} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/70ae59c616a8f0b948c143a29027cbf7a8872851 Cr-Commit-Position: refs/heads/master@{#398434}
Message was sent while issue was closed.
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2008843003/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2008843003/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:86: this.$.pageContainer.style.height = isSubpage ? '100%' : ''; this resets the scrollTop of this.$.pageContainer to 0 |
