Merge lp:~jcsackett/juju-gui/scroll-to-top-on-change into lp:juju-gui/experimental

[j.c.sackett]

Resolves views rendering in partly scrolled state

This branch resolves two bugs wherein a view rendered scrolled partway down.
This appears to be an issue with browser behavior rather than JS, as
investigation showed no problem when debuggers were used to check state as the
views render.

To get around that, this branch simply grabs the top-most visible element of
the two views and ensures they're scrolled into view.

https://codereview.appspot.com/9088043/

[j.c.sackett (jcsackett)]

Reviewers: mp+161894_code.launchpad.net,

Message:
Please take a look.

Description:
Resolves views rendering in partly scrolled state

This branch resolves two bugs wherein a view rendered scrolled partway
down.
This appears to be an issue with browser behavior rather than JS, as
investigation showed no problem when debuggers were used to check state
as the
views render.

To get around that, this branch simply grabs the top-most visible
element of
the two views and ensures they're scrolled into view.

https://code.launchpad.net/~jcsackett/juju-gui/scroll-to-top-on-change/+merge/161894

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/9088043/

Affected files:
   A [revision details]
   M app/subapps/browser/views/charm.js
   M app/subapps/browser/views/editorial.js
   M app/subapps/browser/views/search.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: app/subapps/browser/views/charm.js
=== modified file 'app/subapps/browser/views/charm.js'
--- app/subapps/browser/views/charm.js 2013-05-01 07:55:53 +0000
+++ app/subapps/browser/views/charm.js 2013-05-01 17:20:47 +0000
@@ -444,7 +444,6 @@

        var tplData = charm.getAttrs(),
            container = this.get('container');
-
        tplData.isFullscreen = isFullscreen;
        tplData.prettyCommits = this._formatCommitsForHtml(
            tplData.recent_commits);
@@ -463,7 +462,8 @@

        // Set the content then update the container so that it reload
        // events.
- this.get('renderTo').setHTML(tplNode);
+ var viewData = Y.one('.bws-view-data');
+ viewData.setHTML(tplNode);

        this.tabview = new widgets.browser.TabView({
          srcNode: tplNode.one('.tabs')
@@ -481,6 +481,9 @@
        } else {
          this._noReadme(tplNode.one('#bws-readme'));
        }
+ // Scroll the nav bar into view, so we load the charm view at the
top of
+ // the content.
+ viewData.one('.nav').scrollIntoView();
      },

      /**

Index: app/subapps/browser/views/editorial.js
=== modified file 'app/subapps/browser/views/editorial.js'
--- app/subapps/browser/views/editorial.js 2013-04-30 16:06:00 +0000
+++ app/subapps/browser/views/editorial.js 2013-05-01 17:20:47 +0000
@@ -96,7 +96,7 @@
       * @param {Object} request the original io_request object for
debugging.
       */
      apiFailure: function(data, request) {
- this._apiFailure(data, request, 'Failed to load sidebar content.');
+ this._apiFailure(data, request, 'Failed to load editorial content.');
      },

      /**

Index: app/subapps/browser/views/search.js
=== modified file 'app/subapps/browser/views/search.js'
--- app/subapps/browser/views/search.js 2013-04-30 15:25:49 +0000
+++ app/subapps/browser/views/search.js 2013-05-01 17:20:47 +0000
@@ -135,6 +135,9 @@
        this._renderFilterWidget(filter_container);
        this.get('container').setHTML(tplNode);
       ...

[j.c.sackett (jcsackett)]

https://codereview.appspot.com/9088043/diff/1/app/subapps/browser/views/editorial.js
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/9088043/diff/1/app/subapps/browser/views/editorial.js#newcode99
app/subapps/browser/views/editorial.js:99: this._apiFailure(data,
request, 'Failed to load editorial content.');
This was just a driveby.

https://codereview.appspot.com/9088043/

[Richard Harding (rharding)]

LGTM sans the one request to fix renderTo vs work around it assuming
that was the issue.

https://codereview.appspot.com/9088043/diff/1/app/subapps/browser/views/charm.js
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/9088043/diff/1/app/subapps/browser/views/charm.js#newcode465
app/subapps/browser/views/charm.js:465: var viewData =
Y.one('.bws-view-data');
I'm assuming this is changed because the valueFn was grabbing before the
node was there? Can we change renderTo to work with the getter then and
prevent hard coding the Y.one here? The renderTo was updated to keep all
the views in common and for testing. This happens to work because you're
using the default value anyway.

https://codereview.appspot.com/9088043/

[lp:~jcsackett/juju-gui/scroll-to-top-on-change]

633. By j.c.sackett on 2013-05-01

renderTo instead of viewData

[j.c.sackett (jcsackett)]

https://codereview.appspot.com/9088043/diff/1/app/subapps/browser/views/charm.js
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/9088043/diff/1/app/subapps/browser/views/charm.js#newcode465
app/subapps/browser/views/charm.js:465: var viewData =
Y.one('.bws-view-data');
On 2013/05/01 17:50:06, rharding wrote:
> I'm assuming this is changed because the valueFn was grabbing before
the node
> was there? Can we change renderTo to work with the getter then and
prevent hard
> coding the Y.one here? The renderTo was updated to keep all the views
in common
> and for testing. This happens to work because you're using the default
value
> anyway.

Actually, when I started this line was
Y.one('.bws-view-data').setHTML(tplNode); I think this is a bad
merge--I'm totally happy to use renderTo. I've made that change and
everything works.

https://codereview.appspot.com/9088043/

[Richard Harding (rharding)]

On 2013/05/01 18:49:23, j.c.sackett wrote:

https://codereview.appspot.com/9088043/diff/1/app/subapps/browser/views/charm.js
> File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/9088043/diff/1/app/subapps/browser/views/charm.js#newcode465
> app/subapps/browser/views/charm.js:465: var viewData =
Y.one('.bws-view-data');
> On 2013/05/01 17:50:06, rharding wrote:
> > I'm assuming this is changed because the valueFn was grabbing before
the node
> > was there? Can we change renderTo to work with the getter then and
prevent
> hard
> > coding the Y.one here? The renderTo was updated to keep all the
views in
> common
> > and for testing. This happens to work because you're using the
default value
> > anyway.

> Actually, when I started this line was
> Y.one('.bws-view-data').setHTML(tplNode); I think this is a bad
merge--I'm
> totally happy to use renderTo. I've made that change and everything
works.

Ah, thanks for the follow up. Never mind then.

https://codereview.appspot.com/9088043/

[j.c.sackett (jcsackett)]

Please take a look.

https://codereview.appspot.com/9088043/

[Jeff Pihach (hatch)]

LGTM - however maybe you could put an XXX around those scrollto's with a
small comment as to why they are there so that someone can go and
investigate at a later time

https://codereview.appspot.com/9088043/

[j.c.sackett (jcsackett)]

On 2013/05/01 19:12:40, jeff.pihach wrote:
> LGTM - however maybe you could put an XXX around those scrollto's with
a small
> comment as to why they are there so that someone can go and
investigate at a
> later time
Done.

https://codereview.appspot.com/9088043/

[lp:~jcsackett/juju-gui/scroll-to-top-on-change]

634. By j.c.sackett on 2013-05-01

Make comments on scrollIntoView calls XXX comments.

[j.c.sackett (jcsackett)]

*** Submitted:

Resolves views rendering in partly scrolled state

This branch resolves two bugs wherein a view rendered scrolled partway
down.
This appears to be an issue with browser behavior rather than JS, as
investigation showed no problem when debuggers were used to check state
as the
views render.

To get around that, this branch simply grabs the top-most visible
element of
the two views and ensures they're scrolled into view.

R=rharding, jeff.pihach
CC=
https://codereview.appspot.com/9088043

https://codereview.appspot.com/9088043/

=== modified file 'app/subapps/browser/views/charm.js'
--- app/subapps/browser/views/charm.js	2013-05-01 07:55:53 +0000
+++ app/subapps/browser/views/charm.js	2013-05-01 19:27:25 +0000
@@ -444,7 +444,6 @@
 
       var tplData = charm.getAttrs(),
           container = this.get('container');
-
       tplData.isFullscreen = isFullscreen;
       tplData.prettyCommits = this._formatCommitsForHtml(
           tplData.recent_commits);
@@ -463,7 +462,8 @@
 
       // Set the content then update the container so that it reload
       // events.
-      this.get('renderTo').setHTML(tplNode);
+      var renderTo = this.get('renderTo');
+      renderTo.setHTML(tplNode);
 
       this.tabview = new widgets.browser.TabView({
         srcNode: tplNode.one('.tabs')
@@ -481,6 +481,11 @@
       } else {
         this._noReadme(tplNode.one('#bws-readme'));
       }
+      // XXX: Ideally we shouldn't have to do this; resetting the container
+      // with .empty or something before rendering the charm view should work.
+      // But it doesn't so we scroll the nav bar into view, load the charm
+      // view at the top of the content.
+      renderTo.one('.nav').scrollIntoView();
     },
 
     /**

=== modified file 'app/subapps/browser/views/editorial.js'
--- app/subapps/browser/views/editorial.js	2013-04-30 16:06:00 +0000
+++ app/subapps/browser/views/editorial.js	2013-05-01 19:27:25 +0000
@@ -96,7 +96,7 @@
      * @param {Object} request the original io_request object for debugging.
      */
     apiFailure: function(data, request) {
-      this._apiFailure(data, request, 'Failed to load sidebar content.');
+      this._apiFailure(data, request, 'Failed to load editorial content.');
     },
 
     /**

=== modified file 'app/subapps/browser/views/search.js'
--- app/subapps/browser/views/search.js	2013-04-30 15:25:49 +0000
+++ app/subapps/browser/views/search.js	2013-05-01 19:27:25 +0000
@@ -135,6 +135,11 @@
       this._renderFilterWidget(filter_container);
       this.get('container').setHTML(tplNode);
       target.setHTML(this.get('container'));
+      // XXX: We shouldn't have to do this; calling .empty before rending
+      // should reset where the node's overflow is scrolled to, but it
+      // doesn't. Se we scroll the heading into view to ensure the view
+      // renders at the top of the content.
+      target.one('.search-title').scrollIntoView();
     },
 
     /**