Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2184863002:
MD Settings: Remove paper-header-panel, use IntersectionObserver. (Closed)
|
Created:
4 years, 4 months ago by dpapad Modified:
3 years, 8 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/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Remove paper-header-panel, use IntersectionObserver.
BUG=631281
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2184863002
Cr-Commit-Position: refs/heads/master@{#460865}
Committed: https://chromium.googlesource.com/chromium/src/+/affc1ede3eec8077612c7f7fe76792d57854678f
Patch Set 1 #Patch Set 2 : More #Patch Set 3 : Overlay #Patch Set 4 : Tweaks. #Patch Set 5 : Add CSS only solution, very close. #Patch Set 6 : Nits #Patch Set 7 : Fix test. #Patch Set 8 : Fix test. #
Total comments: 9
Patch Set 9 : Rebase + address comments. #
Total comments: 3
Patch Set 10 : Add TODO #Patch Set 11 : resolve conflicts #
Messages
Total messages: 35 (26 generated)
Description was changed from ========== MD Settings: Remove paper-header-panel, experiment. BUG=631281 ========== to ========== MD Settings: Remove paper-header-panel, experiment. BUG=631281 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Remove paper-header-panel, experiment. BUG=631281 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Remove paper-header-panel, use CSS only waterfall effect. BUG=631281 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MD Settings: Remove paper-header-panel, use CSS only waterfall effect. BUG=631281 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Remove paper-header-panel, use IntersectionObserver. BUG=631281 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
scottchen@chromium.org changed reviewers: + scottchen@chromium.org
https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:163: intersectionObserver_: null, Do you need @private here?
https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:163: intersectionObserver_: null, On 2017/03/28 at 22:06:55, scottchen wrote: > Do you need @private here? This needs "@private {?IntersectionObserver}", but I have not added it yet, since the compilation fails anyway. Will add once the JS Compiler externs issue is resolved.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:75: } indent off https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:176: this.setupDropShadow_(); just make this part of attached rather than a separate method (just delete L176-180) https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:185: this.$.dropShadow.classList.remove('has-shadow'); this.$.dropShadow.classList.toggle('has-shadow', entries[0].intersectionRatio == 0);
This CL compiles fine now, since it is based on https://codereview.chromium.org/2786793002 which rolls Closure Compiler. https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:75: } On 2017/03/29 at 13:19:55, Dan Beam (slow) wrote: > indent off Done. https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:163: intersectionObserver_: null, On 2017/03/28 at 22:10:57, dpapad wrote: > On 2017/03/28 at 22:06:55, scottchen wrote: > > Do you need @private here? > > This needs "@private {?IntersectionObserver}", but I have not added it yet, since the compilation fails anyway. Will add once the JS Compiler externs issue is resolved. Done. https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:176: this.setupDropShadow_(); On 2017/03/29 at 13:19:55, Dan Beam (slow) wrote: > just make this part of attached rather than a separate method (just delete L176-180) Done. https://codereview.chromium.org/2184863002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:185: this.$.dropShadow.classList.remove('has-shadow'); On 2017/03/29 at 13:19:55, Dan Beam (slow) wrote: > this.$.dropShadow.classList.toggle('has-shadow', entries[0].intersectionRatio == 0); Done.
lgtm https://codereview.chromium.org/2184863002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2184863002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:65: box-shadow: inset 0 5px 6px -3px rgba(0, 0, 0, 0.4); is there a mixin that you can apply instead to get this shadow?
https://codereview.chromium.org/2184863002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2184863002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:65: box-shadow: inset 0 5px 6px -3px rgba(0, 0, 0, 0.4); On 2017/03/29 at 23:29:55, Dan Beam (slow) wrote: > is there a mixin that you can apply instead to get this shadow? Do you mean a CSS variable, instead of a mixin? Currently there isn't. This value was copied from paper-header-panel, https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... and I see identical copies in History and Downloads https://cs.chromium.org/chromium/src/chrome/browser/resources/md_history/app.... https://cs.chromium.org/chromium/src/chrome/browser/resources/md_downloads/ma... We can create a CSS variable to unify those literals, as a future cleanup. Added a TODO for this.
still lgtm (you shouldn't need a role="none" on your <paper-header-panel> replacement, btw) https://codereview.chromium.org/2184863002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2184863002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.html:65: box-shadow: inset 0 5px 6px -3px rgba(0, 0, 0, 0.4); On 2017/03/30 01:49:12, dpapad wrote: > On 2017/03/29 at 23:29:55, Dan Beam (slow) wrote: > > is there a mixin that you can apply instead to get this shadow? > > Do you mean a CSS variable, instead of a mixin? no, I meant this: https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... > Currently there isn't. This > value was copied from paper-header-panel, yeah, i see that now :( > > https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... > > and I see identical copies in History and Downloads > https://cs.chromium.org/chromium/src/chrome/browser/resources/md_history/app.... > https://cs.chromium.org/chromium/src/chrome/browser/resources/md_downloads/ma... > > We can create a CSS variable to unify those literals, as a future cleanup. Added > a TODO for this. yeah, that's cool with me
The CQ bit was checked by dpapad@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/2184863002/#ps220001 (title: "resolve conflicts")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490895862847160,
"parent_rev": "faa4d68dd2fec796031ce483ef8aed4929bef34c", "commit_rev":
"affc1ede3eec8077612c7f7fe76792d57854678f"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Remove paper-header-panel, use IntersectionObserver. BUG=631281 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Remove paper-header-panel, use IntersectionObserver. BUG=631281 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2184863002 Cr-Commit-Position: refs/heads/master@{#460865} Committed: https://chromium.googlesource.com/chromium/src/+/affc1ede3eec8077612c7f7fe767... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/affc1ede3eec8077612c7f7fe767... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
