MD Settings: Scroll to top when navigating to About page.

MD Settings: Scroll to top when navigating to About page.

BUG=667671
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2627403003
Cr-Commit-Position: refs/heads/master@{#444217}
Committed: https://chromium.googlesource.com/chromium/src/+/52d38e7a5ae31b750d87205b072a324cf99926ef

[tommycli, 2017-01-13 19:53:16]

Description was changed from

==========
MD Settings: Scroll to top when navigating to About page.

BUG=667671
==========

to

==========
MD Settings: Scroll to top when navigating to About page.

BUG=667671
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2017-01-13 19:54:07]

tommycli@chromium.org changed reviewers:
+ dbeam@chromium.org

[tommycli, 2017-01-13 19:54:07]

dbeam: PTAL, thanks!

[tommycli, 2017-01-17 22:25:24]

On 2017/01/13 19:54:07, tommycli wrote:
> dbeam: PTAL, thanks!

dbeam: ping, thanks!

[tommycli, 2017-01-17 23:07:23]

tommycli@chromium.org changed reviewers:
+ dschuyler@chromium.org

[tommycli, 2017-01-17 23:07:23]

dschuyler: PTAL, thanks!

[dschuyler, 2017-01-17 23:33:38]

https://codereview.chromium.org/2627403003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2627403003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:51: //
Always scroll to the top when navigating to the About page.
I'm not sure about this, but WDYT about combining with the condition above?

// Always scroll to the top if navigating from a section to the root route
// or when navigating to the About page.
if ((oldRouteWasSection && newRoute == settings.Route.BASIC) ||
    (newRoute == settings.Route.ABOUT && this.scroller)) {
  ...
}

My concern is that if something about the body of the code needs to change in
the future, the person making that change may miss that both blocks do the same
thing and only change one of them.
If you feel that it's better as two separate condition blocks, please let me
know.

https://codereview.chromium.org/2627403003/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/basic_page_browsertest.js (right):

https://codereview.chromium.org/2627403003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/basic_page_browsertest.js:176:
settings.navigateTo(settings.Route.ABOUT);
Could this need a Polymer.dom.flush() or something? It may be fine, I'm just
worried this may fail if a test bot hitches or something.

[tommycli, 2017-01-17 23:47:00]

dschuyler: Thanks! I took the suggestion.

https://codereview.chromium.org/2627403003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2627403003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:51: //
Always scroll to the top when navigating to the About page.
On 2017/01/17 23:33:38, dschuyler wrote:
> I'm not sure about this, but WDYT about combining with the condition above?
> 
> // Always scroll to the top if navigating from a section to the root route
> // or when navigating to the About page.
> if ((oldRouteWasSection && newRoute == settings.Route.BASIC) ||
>     (newRoute == settings.Route.ABOUT && this.scroller)) {
>   ...
> }
> 
> My concern is that if something about the body of the code needs to change in
> the future, the person making that change may miss that both blocks do the
same
> thing and only change one of them.
> If you feel that it's better as two separate condition blocks, please let me
> know. 

Done.

https://codereview.chromium.org/2627403003/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/basic_page_browsertest.js (right):

https://codereview.chromium.org/2627403003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/basic_page_browsertest.js:176:
settings.navigateTo(settings.Route.ABOUT);
On 2017/01/17 23:33:38, dschuyler wrote:
> Could this need a Polymer.dom.flush() or something? It may be fine, I'm just
> worried this may fail if a test bot hitches or something.

Ah, I don't think it actually needs the flush. How about I add the flush() if
this fails CQ / we see test bots flaking on this?

[dschuyler, 2017-01-17 23:53:59]

lgtm

[tommycli, 2017-01-17 23:57:25]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-01-17 23:57:31]

On 2017/01/17 23:53:59, dschuyler wrote:
> lgtm

thanks!

[commit-bot: I haz the power, 2017-01-17 23:57:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 01:03:40]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484697445691280,
"parent_rev": "8525c81d710cd9a5f92c343825094345e333afd3", "commit_rev":
"52d38e7a5ae31b750d87205b072a324cf99926ef"}

[commit-bot: I haz the power, 2017-01-18 01:04:11]

Description was changed from

==========
MD Settings: Scroll to top when navigating to About page.

BUG=667671
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Scroll to top when navigating to About page.

BUG=667671
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2627403003
Cr-Commit-Position: refs/heads/master@{#444217}
Committed:
https://chromium.googlesource.com/chromium/src/+/52d38e7a5ae31b750d87205b072a...
==========

[commit-bot: I haz the power, 2017-01-18 01:04:13]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/52d38e7a5ae31b750d87205b072a...