Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2076193003:
[MD settings] Remove old side nav code; remove extra toolbar (Closed)
|
Created:
4 years, 6 months ago by dschuyler Modified:
4 years, 6 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+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. |
Description[MD settings] Remove old side nav code; remove extra toolbar
This CL effectively reverts CLs 1060983004 and 1039373002. The side nav
toolbar is exchanged for a simpler div and the on menu select event is
changed for an on activate which has better scope for the usage.
BUG=593989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/ebe571aa0319d7588b2ef12d7917732a6e7b24ac
Cr-Commit-Position: refs/heads/master@{#400554}
Patch Set 1 #
Total comments: 8
Patch Set 2 : review changes #
Total comments: 2
Patch Set 3 : reveiw nit #
Total comments: 8
Patch Set 4 : review changes #
Dependent Patchsets: Messages
Total messages: 21 (6 generated)
Description was changed from ========== [MD settings] Remove old side nav code; remove extra toolbar This CL effectively reverts CLs 1060983004 and 1039373002. The side nav toolbar is exchanged for a simpler div and the on menu select event is changed for an on activate which has better scope for the usage. BUG=593989 ========== to ========== [MD settings] Remove old side nav code; remove extra toolbar This CL effectively reverts CLs 1060983004 and 1039373002. The side nav toolbar is exchanged for a simpler div and the on menu select event is changed for an on activate which has better scope for the usage. BUG=593989 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
dschuyler@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_ui/settings_ui.html:161: <div class="drawer-toolbar"> Shouldn't this be class="paper-header" per documentation at https://elements.polymer-project.org/elements/paper-header-panel? I am surprised this div is even displayed since it does not match any content selector within paper-header-panel. https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_ui/settings_ui.html:162: <div>$i18n{settings}</div> Do we need the two divs? Can they just be merged into one?
https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_ui/settings_ui.html:161: <div class="drawer-toolbar"> On 2016/06/17 at 21:38:46, dpapad wrote: > Shouldn't this be class="paper-header" per documentation at https://elements.polymer-project.org/elements/paper-header-panel? I am surprised this div is even displayed since it does not match any content selector within paper-header-panel. I actually understand why it is displayed, but I think without paper-header CSS class, it will not be visible when you start scrolling the contents of this paper-header-panel.
https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_ui/settings_ui.html:161: <div class="drawer-toolbar"> On 2016/06/17 21:40:52, dpapad wrote: > On 2016/06/17 at 21:38:46, dpapad wrote: > > Shouldn't this be class="paper-header" per documentation at > https://elements.polymer-project.org/elements/paper-header-panel I am surprised > this div is even displayed since it does not match any content selector within > paper-header-panel. > > I actually understand why it is displayed, but I think without paper-header CSS > class, it will not be visible when you start scrolling the contents of this > paper-header-panel. Acknowledged. https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_ui/settings_ui.html:161: <div class="drawer-toolbar"> On 2016/06/17 21:38:46, dpapad wrote: > Shouldn't this be class="paper-header" per documentation at > https://elements.polymer-project.org/elements/paper-header-panel I am surprised > this div is even displayed since it does not match any content selector within > paper-header-panel. It's up to Alan, but I'll add it now and he can review it later. I also added mode waterfall to the header panel. Done. https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_ui/settings_ui.html:162: <div>$i18n{settings}</div> On 2016/06/17 21:38:46, dpapad wrote: > Do we need the two divs? Can they just be merged into one? Done.
LGTM. Thanks! https://codereview.chromium.org/2076193003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2076193003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:158: <div class="paper-header drawer-toolbar">$i18n{settings}</div> Nit (optional): How about removing drawer-toolbar class completely, and rename line 137 to .paper-header?
https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_ui/settings_ui.html:161: <div class="drawer-toolbar"> > It's up to Alan, but I'll add it now and he can > review it later. I also added mode waterfall to the header panel. > > Done. Is the waterfall effect very notice-able? I would lean towards leaving it as-is if Alan has not mentioned anything about it so far, and only change it after we bring it to his attention.
https://codereview.chromium.org/2076193003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2076193003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:158: <div class="paper-header drawer-toolbar">$i18n{settings}</div> On 2016/06/17 22:21:20, dpapad wrote: > Nit (optional): How about removing drawer-toolbar class completely, and rename > line 137 to .paper-header? Done.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_ui/settings_ui.html:161: <div class="drawer-toolbar"> On 2016/06/17 22:24:04, dpapad wrote: > > > It's up to Alan, but I'll add it now and he can > > review it later. I also added mode waterfall to the header panel. > > > > Done. > > Is the waterfall effect very notice-able? I would lean towards leaving it as-is > if Alan has not mentioned anything about it so far, and only change it after we > bring it to his attention. alan wants waterfall
On 2016/06/17 at 22:28:22, dbeam wrote: > https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): > > https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/settings_ui/settings_ui.html:161: <div class="drawer-toolbar"> > On 2016/06/17 22:24:04, dpapad wrote: > > > > > It's up to Alan, but I'll add it now and he can > > > review it later. I also added mode waterfall to the header panel. > > > > > > Done. > > > > Is the waterfall effect very notice-able? I would lean towards leaving it as-is > > if Alan has not mentioned anything about it so far, and only change it after we > > bring it to his attention. > > alan wants waterfall Thanks for clarifying this. Summarizing offline conversation with Dave it seems that Alan wants waterfall for the main page, but we are not sure if waterfall is also desired for the drawer, so there is a small assumption being made for the drawer with this CL.
lgtm https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (left): https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:164: flex-shrink: 0; you may want to leave this this is what make the "Settings" title stay the same size even when the window gets smaller, right? https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:140: align-items: center; alphabetize (did you bypass a presubmit?) https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:163: <paper-toolbar class="toolbar"> is this class="toolbar" still doing anything? https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:38: onIronActivate_: function(e) { nit: if you don't use |e|, arguably remove it and the @param
On 2016/06/17 23:15:10, dpapad wrote: > On 2016/06/17 at 22:28:22, dbeam wrote: > > > https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... > > File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): > > > > > https://codereview.chromium.org/2076193003/diff/1/chrome/browser/resources/se... > > chrome/browser/resources/settings/settings_ui/settings_ui.html:161: <div > class="drawer-toolbar"> > > On 2016/06/17 22:24:04, dpapad wrote: > > > > > > > It's up to Alan, but I'll add it now and he can > > > > review it later. I also added mode waterfall to the header panel. > > > > > > > > Done. > > > > > > Is the waterfall effect very notice-able? I would lean towards leaving it > as-is > > > if Alan has not mentioned anything about it so far, and only change it after > we > > > bring it to his attention. > > > > alan wants waterfall > > Thanks for clarifying this. Summarizing offline conversation with Dave it seems > that Alan wants waterfall for the main page, but we are not sure if waterfall is > also desired for the drawer, so there is a small assumption being made for the > drawer with this CL. yeah, i don't know what alan wants there, then. this is only if the drawer itself scrolls? he'd probably want waterfall for consistency, but i think you're free to do whatever here (as far as I'm concerned)
https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (left): https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:164: flex-shrink: 0; On 2016/06/17 23:38:38, Dan Beam wrote: > you may want to leave this > > this is what make the "Settings" title stay the same size even when the window > gets smaller, right? I added a comment about that as well. Done. https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:140: align-items: center; On 2016/06/17 23:38:38, Dan Beam wrote: > alphabetize (did you bypass a presubmit?) I still have the output in my console: Presubmit checks passed, so maybe something is amiss there. Done. https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:163: <paper-toolbar class="toolbar"> On 2016/06/17 23:38:38, Dan Beam wrote: > is this class="toolbar" still doing anything? Done. https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2076193003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:38: onIronActivate_: function(e) { On 2016/06/17 23:38:38, Dan Beam wrote: > nit: if you don't use |e|, arguably remove it and the @param Done.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2076193003/#ps60001 (title: "review changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2076193003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [MD settings] Remove old side nav code; remove extra toolbar This CL effectively reverts CLs 1060983004 and 1039373002. The side nav toolbar is exchanged for a simpler div and the on menu select event is changed for an on activate which has better scope for the usage. BUG=593989 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] Remove old side nav code; remove extra toolbar This CL effectively reverts CLs 1060983004 and 1039373002. The side nav toolbar is exchanged for a simpler div and the on menu select event is changed for an on activate which has better scope for the usage. BUG=593989 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/ebe571aa0319d7588b2ef12d7917732a6e7b24ac Cr-Commit-Position: refs/heads/master@{#400554} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ebe571aa0319d7588b2ef12d7917732a6e7b24ac Cr-Commit-Position: refs/heads/master@{#400554} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
