Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2074063003:
MD Settings: Migrate from paper-drawer-panel to app-drawer. (Closed)
|
Created:
4 years, 6 months ago by dpapad Modified:
4 years, 6 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-elements_chromium.org, dbeam+watch-polymer_chromium.org, dbeam+watch-settings_chromium.org, Dan Beam, michaelpg+watch-md-ui_chromium.org, michaelpg+watch-elements_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-polymer_chromium.org, oshima+watch_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. |
DescriptionMD Settings: Migrate from paper-drawer-panel to app-drawer.
Also remove usage of paper-toolbar replace with an equivalent simple
div for now. The goal is to re-use cr-toolbar in follow up CLs.
As a result of using app-drawer two bugs are fixed. Previously
- user could interact with the search box while the drawer was open,
- user could not close drawer by clicking on the toolbar
BUG=608535, 602896
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/37e9a8a91b3ec7abfb226cd63553a2149ef0888f
Cr-Commit-Position: refs/heads/master@{#400854}
Patch Set 1 : More work. #Patch Set 2 : Rebase #
Total comments: 4
Patch Set 3 : Remove some resources. #
Total comments: 2
Patch Set 4 : Fixing RTL issue. #
Total comments: 9
Patch Set 5 : Addressing comments. #
Total comments: 4
Patch Set 6 : Addressing comments #
Dependent Patchsets: Messages
Total messages: 35 (19 generated)
Description was changed from ========== WIP, Port cr-toolbar. BUG= ========== to ========== WIP, Port cr-toolbar. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== WIP, Port cr-toolbar. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== WIP, Port app-drawer, cr-toolbar. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== WIP, Port app-drawer, cr-toolbar. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. The app-layout family of elements is the new focus of Polymer. This also fixes two bugs - user can interact with the search box while the drawer in open, - drawer does not close when clicking on the toolbar BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. The app-layout family of elements is the new focus of Polymer. This also fixes two bugs - user can interact with the search box while the drawer in open, - drawer does not close when clicking on the toolbar BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. Also remove usage of paper-toolbar replace with an equivalent simple div for now. The goal is to re-use cr-toolbar in follow up CLs. This also fixes two bugs - user can interact with the search box while the drawer in open, - drawer does not close when clicking on the toolbar BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. Also remove usage of paper-toolbar replace with an equivalent simple div for now. The goal is to re-use cr-toolbar in follow up CLs. This also fixes two bugs - user can interact with the search box while the drawer in open, - drawer does not close when clicking on the toolbar BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. Also remove usage of paper-toolbar replace with an equivalent simple div for now. The goal is to re-use cr-toolbar in follow up CLs. As a result of using app-drawer two bugs are fixed - user can interact with the search box while the drawer in open, - drawer does not close when clicking on the toolbar BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
dpapad@chromium.org changed reviewers: + dschuyler@chromium.org, michaelpg@chromium.org
Screenshots at http://imgur.com/a/oTP3Z. The RTL issue (see related comment), needs to be resolved before this CL can land. https://codereview.chromium.org/2074063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2074063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:154: <paper-header-panel mode="waterfall"> As explained in today's standup, it seems that there are more than one places where <settings-main> assumes it lives within a paper-header-panel, so I could not remove this yet. https://codereview.chromium.org/2074063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2074063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:41: //this.$.panel.rightDrawer = this.directionDelegate.isRtl(); This should no longer be necessary, because app-drawer supports align="start" which should work for both RTL and LTR according to docs [1], but there seems to be a bug at [2]. Seems like a race condition where at the time it is called during startup returns LTR, and only changes to RTL after it is no longer called. Needs further investigation. Filed https://github.com/PolymerElements/app-layout/issues/247. [1] https://elements.polymer-project.org/elements/app-layout?active=app-drawer#pr... [2] https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...
https://codereview.chromium.org/2074063003/diff/120001/ui/webui/resources/pol... File ui/webui/resources/polymer_resources.grdp (right): https://codereview.chromium.org/2074063003/diff/120001/ui/webui/resources/pol... ui/webui/resources/polymer_resources.grdp:124: type="chrome_html" /> Wow. Are all of these necessary?
https://codereview.chromium.org/2074063003/diff/120001/ui/webui/resources/pol... File ui/webui/resources/polymer_resources.grdp (right): https://codereview.chromium.org/2074063003/diff/120001/ui/webui/resources/pol... ui/webui/resources/polymer_resources.grdp:124: type="chrome_html" /> On 2016/06/20 at 17:21:00, michaelpg wrote: > Wow. Are all of these necessary? No, removed all but app-drawer. I was initially also using app-toolbar, but ended up using a simple div (and planning to swap with cr-toolbar in follow up CL).
Fixed, RTL issue per suggestion at https://github.com/PolymerElements/app-layout/issues/247#issuecomment-227213021 (use right/left instead of start/end).
Description was changed from ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. Also remove usage of paper-toolbar replace with an equivalent simple div for now. The goal is to re-use cr-toolbar in follow up CLs. As a result of using app-drawer two bugs are fixed - user can interact with the search box while the drawer in open, - drawer does not close when clicking on the toolbar BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. Also remove usage of paper-toolbar replace with an equivalent simple div for now. The goal is to re-use cr-toolbar in follow up CLs. As a result of using app-drawer two bugs are fixed. Previously - user could interact with the search box while the drawer in open, - user could not close drawer by clicking on the toolbar BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. Also remove usage of paper-toolbar replace with an equivalent simple div for now. The goal is to re-use cr-toolbar in follow up CLs. As a result of using app-drawer two bugs are fixed. Previously - user could interact with the search box while the drawer in open, - user could not close drawer by clicking on the toolbar BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. Also remove usage of paper-toolbar replace with an equivalent simple div for now. The goal is to re-use cr-toolbar in follow up CLs. As a result of using app-drawer two bugs are fixed. Previously - user could interact with the search box while the drawer was open, - user could not close drawer by clicking on the toolbar BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2074063003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2074063003/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:7: <link rel="import" href="chrome://resources/polymer/v1_0/app-layout/app-drawer/app-drawer.html"> alphabetize https://codereview.chromium.org/2074063003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2074063003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:105: font-size: 123.08%; /* go to 16px from 13px */ hmm, dbeam just used 1.23077rem somewhere else. any difference between those? https://codereview.chromium.org/2074063003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:138: </paper-icon-button> no indent https://codereview.chromium.org/2074063003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2074063003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:45: onMenuButtonTap_: function() { private https://codereview.chromium.org/2074063003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:45: onMenuButtonTap_: function() { opt nit: move up to group onEvent fns together https://codereview.chromium.org/2074063003/diff/160001/ui/webui/resources/pol... File ui/webui/resources/polymer_resources.grdp (right): https://codereview.chromium.org/2074063003/diff/160001/ui/webui/resources/pol... ui/webui/resources/polymer_resources.grdp:24: file="../../../third_party/polymer/v1_0/components-chromium/app-layout/app-drawer/app-drawer-extracted.js" remove app-layout from find_unused_elements.py WHITELIST
> BUG=608535 not quite the right bug, is there a more relevant one?
https://codereview.chromium.org/2074063003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2074063003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:105: font-size: 123.08%; /* go to 16px from 13px */ On 2016/06/20 19:43:59, michaelpg wrote: > hmm, dbeam just used 1.23077rem somewhere else. any difference between those? p.s. I was wrong
On 2016/06/20 at 19:44:55, michaelpg wrote: > > BUG=608535 > not quite the right bug, is there a more relevant one? There is not a more relevant bug on the tracker as of now. I listed this CL under 608535, since my motivation of doing this is to get the search toolbar working per spec. Perhaps crbug.com/602896 is more relevant?
https://codereview.chromium.org/2074063003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2074063003/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:7: <link rel="import" href="chrome://resources/polymer/v1_0/app-layout/app-drawer/app-drawer.html"> On 2016/06/20 at 19:43:59, michaelpg wrote: > alphabetize Done. https://codereview.chromium.org/2074063003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2074063003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:138: </paper-icon-button> On 2016/06/20 at 19:43:59, michaelpg wrote: > no indent Done. https://codereview.chromium.org/2074063003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2074063003/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:45: onMenuButtonTap_: function() { On 2016/06/20 at 19:43:59, michaelpg wrote: > private Done. https://codereview.chromium.org/2074063003/diff/160001/ui/webui/resources/pol... File ui/webui/resources/polymer_resources.grdp (right): https://codereview.chromium.org/2074063003/diff/160001/ui/webui/resources/pol... ui/webui/resources/polymer_resources.grdp:24: file="../../../third_party/polymer/v1_0/components-chromium/app-layout/app-drawer/app-drawer-extracted.js" On 2016/06/20 at 19:43:59, michaelpg wrote: > remove app-layout from find_unused_elements.py WHITELIST Done.
LGTM with TODO https://codereview.chromium.org/2074063003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2074063003/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:132: <div class="toolbar"> Please add a todo here that talks about needing to add a waterfall effect. Or a todo with a link to a bug about following up with bettes@ about whether there should be a waterfall effect here :)
On 2016/06/20 20:26:37, dpapad wrote: > On 2016/06/20 at 19:44:55, michaelpg wrote: > > > BUG=608535 > > not quite the right bug, is there a more relevant one? > > There is not a more relevant bug on the tracker as of now. I listed this CL > under 608535, since my motivation of doing this is to get the search toolbar > working per spec. Perhaps crbug.com/602896 is more relevant? crbug.com/602896 SGTM
lgtm https://codereview.chromium.org/2074063003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2074063003/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:4: <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/classes/iron-flex-layout.html"> this file is deprecated, is it unused here? (the --layout-fit definition is in iron-flex-layout/iron-flex-layout.html, so you could import that instead)
https://codereview.chromium.org/2074063003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2074063003/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:4: <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/classes/iron-flex-layout.html"> On 2016/06/20 at 23:30:06, michaelpg wrote: > this file is deprecated, is it unused here? > > (the --layout-fit definition is in iron-flex-layout/iron-flex-layout.html, so you could import that instead) Done, importing iron-flex-layout/iron-flex-layout.html instead. https://codereview.chromium.org/2074063003/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:132: <div class="toolbar"> On 2016/06/20 at 23:11:58, dschuyler wrote: > Please add a todo here that talks > about needing to add a waterfall effect. > > Or a todo with a link to a bug about following > up with bettes@ about whether there > should be a waterfall effect here :) Done. Added the TODO within app-drawer a few lines below, since the effect in question is the one in the drawer. The waterfall effect of the page is preserved.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2074063003/#ps200001 (title: "Addressing comments")
The CQ bit was unchecked by dpapad@chromium.org
Description was changed from ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. Also remove usage of paper-toolbar replace with an equivalent simple div for now. The goal is to re-use cr-toolbar in follow up CLs. As a result of using app-drawer two bugs are fixed. Previously - user could interact with the search box while the drawer was open, - user could not close drawer by clicking on the toolbar BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. Also remove usage of paper-toolbar replace with an equivalent simple div for now. The goal is to re-use cr-toolbar in follow up CLs. As a result of using app-drawer two bugs are fixed. Previously - user could interact with the search box while the drawer was open, - user could not close drawer by clicking on the toolbar BUG=608535,602896 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2074063003/200001
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. Also remove usage of paper-toolbar replace with an equivalent simple div for now. The goal is to re-use cr-toolbar in follow up CLs. As a result of using app-drawer two bugs are fixed. Previously - user could interact with the search box while the drawer was open, - user could not close drawer by clicking on the toolbar BUG=608535,602896 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrate from paper-drawer-panel to app-drawer. Also remove usage of paper-toolbar replace with an equivalent simple div for now. The goal is to re-use cr-toolbar in follow up CLs. As a result of using app-drawer two bugs are fixed. Previously - user could interact with the search box while the drawer was open, - user could not close drawer by clicking on the toolbar BUG=608535,602896 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/37e9a8a91b3ec7abfb226cd63553a2149ef0888f Cr-Commit-Position: refs/heads/master@{#400854} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/37e9a8a91b3ec7abfb226cd63553a2149ef0888f Cr-Commit-Position: refs/heads/master@{#400854} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
