Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2206723002:
[MD settings] shrink overscroll when scrolling up (Closed)
|
Created:
4 years, 4 months ago by dschuyler Modified:
4 years, 4 months ago Reviewers:
Dan Beam 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] shrink overscroll when scrolling up
This CL will shrink the overscroll height at the bottom of the page
while scrolling the content. It also adds a minimum height for the
overscroll to allow space for the advanced page content to be seen when
the Advanced Toggle is used.
BUG=593989, 631249
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/46ffd77c225d30b65092545efe2712589e611601
Cr-Commit-Position: refs/heads/master@{#409669}
Patch Set 1 : changed var name #
Total comments: 6
Patch Set 2 : review changes #Patch Set 3 : review changes #
Total comments: 4
Patch Set 4 : review changes #
Total comments: 8
Patch Set 5 : merge with master #
Total comments: 2
Messages
Total messages: 47 (32 generated)
Description was changed from ========== [MD settings] shrink overscroll when scrolling up This CL will shrink the overscroll height at the bottom of the page while scrolling the content. It also adds a minimum height for the overscroll to allow space for the advanced page content to be seen when the Advanced Toggle is used. BUG=593989, 631249 ========== to ========== [MD settings] shrink overscroll when scrolling up This CL will shrink the overscroll height at the bottom of the page while scrolling the content. It also adds a minimum height for the overscroll to allow space for the advanced page content to be seen when the Advanced Toggle is used. BUG=593989, 631249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@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 checked by dschuyler@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...
Patchset #1 (id:1) has been deleted
Description was changed from ========== [MD settings] shrink overscroll when scrolling up This CL will shrink the overscroll height at the bottom of the page while scrolling the content. It also adds a minimum height for the overscroll to allow space for the advanced page content to be seen when the Advanced Toggle is used. BUG=593989, 631249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] shrink overscroll when scrolling up This CL will shrink the overscroll height at the bottom of the page while scrolling the content. It also adds a minimum height for the overscroll to allow space for the advanced page content to be seen when the Advanced Toggle is used. BUG=593989, 631249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dschuyler@chromium.org changed reviewers: + dbeam@chromium.org
i don't really understand what's going on here, but maybe i could see a demo? https://codereview.chromium.org/2206723002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2206723002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:78: MINIMUM_OVERSCROLL: 64, can you make this private? https://codereview.chromium.org/2206723002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:91: let overscrollBottom = this.$.overscroll.offsetTop + nit: var overscroll = this.$.overscroll; (and re-use) https://codereview.chromium.org/2206723002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:95: (overscrollBottom - visibleBottom); indent off
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@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 checked by dschuyler@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...
https://codereview.chromium.org/2206723002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2206723002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:78: MINIMUM_OVERSCROLL: 64, On 2016/08/02 21:57:36, Dan Beam wrote: > can you make this private? Done. And then ended up removing it. https://codereview.chromium.org/2206723002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:91: let overscrollBottom = this.$.overscroll.offsetTop + On 2016/08/02 21:57:36, Dan Beam wrote: > nit: var overscroll = this.$.overscroll; (and re-use) Done. https://codereview.chromium.org/2206723002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:95: (overscrollBottom - visibleBottom); On 2016/08/02 21:57:36, Dan Beam wrote: > indent off Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2206723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2206723002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:82: var mainContainer = this.domHost.$$('paper-header-panel').$.mainContainer; can we not poke into paper-header-panel's local DOM? https://codereview.chromium.org/2206723002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:93: if (visibleOverscroll > 0) so why are we only setting this when overscroll > 0? shouldn't we just be doing Math.min(0, visibleOverscroll)?
The CQ bit was checked by dschuyler@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...
https://codereview.chromium.org/2206723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2206723002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:82: var mainContainer = this.domHost.$$('paper-header-panel').$.mainContainer; On 2016/08/03 01:30:09, Dan Beam wrote: > can we not poke into paper-header-panel's local DOM? I changed it to something a bit more direct. I'm not sure how to get the scroll event and data without looking at the #mainContainer. Done. https://codereview.chromium.org/2206723002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:93: if (visibleOverscroll > 0) On 2016/08/03 01:30:09, Dan Beam wrote: > so why are we only setting this when overscroll > 0? shouldn't we just be doing > Math.min(0, visibleOverscroll)? Done. That would be logically the same, but function calls have a high overhead compared to the conditional. Since it was logically the same and equally readable, I was going for the one with less overhead.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2206723002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2206723002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:82: var mainContainer = this.parentNode.$.mainContainer; can we just use this.parentNode.scroller? https://codereview.chromium.org/2206723002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:84: mainContainer.addEventListener('scroll', function() { can we only listen to scroll when there is padding to remove? https://codereview.chromium.org/2206723002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:87: // occurs (relative to the scroll action). ^ you can remove this now, I think
i might do it like this https://codereview.chromium.org/2206723002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2206723002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:87: <div id="overscroll"></div> properties: { overscroll_: { type: Number, observer: 'overscrollChanged_' }, } /** @private */ overscrollChanged_: function() { if (!this.overscroll_ && this.boundScroll_) { this.parentNode.scroller.removeEventListener('scroll', this.boundScroll_) this.boundScroll_ = null; } else if (this.overscroll_ && !this.boundScroll_) { this.boundScroll_ = this.onScroll_.bind(this); this.parentNode.scroller.addEventListener('scroll', this.boundScroll_); } }, /** @private */ onScroll_: function() { // the calcs this.overscroll_ = ...; // NB: 10 rather than '10px' }, and in the HTML: <div id="overscroll" style="padding-bottom: [[overscroll]]px"></div> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The CQ bit was checked by dschuyler@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Patchset #5 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2206723002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/2206723002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.html:87: <div id="overscroll"></div> On 2016/08/03 20:57:39, Dan Beam wrote: > properties: { > overscroll_: { > type: Number, > observer: 'overscrollChanged_' > }, > } > > /** @private */ > overscrollChanged_: function() { > if (!this.overscroll_ && this.boundScroll_) { > this.parentNode.scroller.removeEventListener('scroll', this.boundScroll_) > this.boundScroll_ = null; > } else if (this.overscroll_ && !this.boundScroll_) { > this.boundScroll_ = this.onScroll_.bind(this); > this.parentNode.scroller.addEventListener('scroll', this.boundScroll_); > } > }, > > /** @private */ > onScroll_: function() { > // the calcs > this.overscroll_ = ...; // NB: 10 rather than '10px' > }, > > and in the HTML: > > <div id="overscroll" style="padding-bottom: [[overscroll]]px"></div> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Done. https://codereview.chromium.org/2206723002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2206723002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:82: var mainContainer = this.parentNode.$.mainContainer; On 2016/08/03 20:50:32, Dan Beam wrote: > can we just use this.parentNode.scroller? Done. https://codereview.chromium.org/2206723002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:84: mainContainer.addEventListener('scroll', function() { On 2016/08/03 20:50:32, Dan Beam wrote: > can we only listen to scroll when there is padding to remove? Done. https://codereview.chromium.org/2206723002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:87: // occurs (relative to the scroll action). On 2016/08/03 20:50:32, Dan Beam wrote: > ^ you can remove this now, I think Done.
lgtm worth writing tests or do we want to make sure overscroll is here to stay first? https://codereview.chromium.org/2206723002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2206723002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:110: this.overscroll_ = Math.max(0, visibleOverscroll); if this could share more with overscrollHeight_ that's cool, i.e. why can't this be more similar to this.overscroll_ = this.overscrollHeight_(); ?
I can work on tests for it next. https://codereview.chromium.org/2206723002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2206723002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:110: this.overscroll_ = Math.max(0, visibleOverscroll); On 2016/08/03 22:04:59, Dan Beam wrote: > if this could share more with overscrollHeight_ that's cool, i.e. why can't this > be more similar to > > this.overscroll_ = this.overscrollHeight_(); > > ? I think the math looks similar off the cuff. I don't think they'd combine well. The common bits appear to be foo: function(a, b, c, d, e) { return Math.max(0, e - ((a + b) - (c + b))); } and then call foo() with two sets of five parameters. Would adding a function like foo() help?
On 2016/08/03 22:26:03, dschuyler wrote: > I can work on tests for it next. > > https://codereview.chromium.org/2206723002/diff/120001/chrome/browser/resourc... > File chrome/browser/resources/settings/settings_main/settings_main.js (right): > > https://codereview.chromium.org/2206723002/diff/120001/chrome/browser/resourc... > chrome/browser/resources/settings/settings_main/settings_main.js:110: > this.overscroll_ = Math.max(0, visibleOverscroll); > On 2016/08/03 22:04:59, Dan Beam wrote: > > if this could share more with overscrollHeight_ that's cool, i.e. why can't > this > > be more similar to > > > > this.overscroll_ = this.overscrollHeight_(); > > > > ? > > I think the math looks similar off the cuff. I don't think > they'd combine well. The common bits appear to be > > foo: function(a, b, c, d, e) { > return Math.max(0, e - ((a + b) - (c + b))); > } > > and then call foo() with two sets of five parameters. Would > adding a function like foo() help? thank you for the analysis. i'd just prototype and reset or commit depending whether i found the prototype better overall. i trust your judgement there.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [MD settings] shrink overscroll when scrolling up This CL will shrink the overscroll height at the bottom of the page while scrolling the content. It also adds a minimum height for the overscroll to allow space for the advanced page content to be seen when the Advanced Toggle is used. BUG=593989, 631249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] shrink overscroll when scrolling up This CL will shrink the overscroll height at the bottom of the page while scrolling the content. It also adds a minimum height for the overscroll to allow space for the advanced page content to be seen when the Advanced Toggle is used. BUG=593989, 631249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [MD settings] shrink overscroll when scrolling up This CL will shrink the overscroll height at the bottom of the page while scrolling the content. It also adds a minimum height for the overscroll to allow space for the advanced page content to be seen when the Advanced Toggle is used. BUG=593989, 631249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] shrink overscroll when scrolling up This CL will shrink the overscroll height at the bottom of the page while scrolling the content. It also adds a minimum height for the overscroll to allow space for the advanced page content to be seen when the Advanced Toggle is used. BUG=593989, 631249 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/46ffd77c225d30b65092545efe2712589e611601 Cr-Commit-Position: refs/heads/master@{#409669} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/46ffd77c225d30b65092545efe2712589e611601 Cr-Commit-Position: refs/heads/master@{#409669}
Message was sent while issue was closed.
On 2016/08/03 22:26:03, dschuyler wrote: > I can work on tests for it next. Please do. We clearly suck at noticing bugs manually *before* the CL lands: crbug.com/634582 > > https://codereview.chromium.org/2206723002/diff/120001/chrome/browser/resourc... > File chrome/browser/resources/settings/settings_main/settings_main.js (right): > > https://codereview.chromium.org/2206723002/diff/120001/chrome/browser/resourc... > chrome/browser/resources/settings/settings_main/settings_main.js:110: > this.overscroll_ = Math.max(0, visibleOverscroll); > On 2016/08/03 22:04:59, Dan Beam wrote: > > if this could share more with overscrollHeight_ that's cool, i.e. why can't > this > > be more similar to > > > > this.overscroll_ = this.overscrollHeight_(); > > > > ? > > I think the math looks similar off the cuff. I don't think > they'd combine well. The common bits appear to be > > foo: function(a, b, c, d, e) { > return Math.max(0, e - ((a + b) - (c + b))); > } > > and then call foo() with two sets of five parameters. Would > adding a function like foo() help? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
