Remove app-scroll-position #417

blasten · 2017-03-01T22:17:19Z

No description provided.

frankiefu

Remove this too:

app-layout/test/index.html

Line 26 in 4dd4304

'../app-scroll-position/test/index.html',

frankiefu · 2017-03-01T22:46:13Z

templates/pesto/src/recipe-app.html

@@ -131,9 +130,6 @@
        </app-header-layout>
      </app-drawer>

-      <app-scroll-position
-          selected="[[_pageData.page]]"
-          reset="[[_isDetailPage(_pageData.page)]]"></app-scroll-position>


Let's add some JS code to save the scroll position so if you navigate back to the list page you are at the previous scroll position. This code will serve as a sample for users for replacing app-scroll-position element.

frankiefu · 2017-03-01T22:49:47Z

templates/publishing/src/blog-app.html

@@ -29,7 +29,6 @@
 <link rel="import" href="../bower_components/app-layout/app-header/app-header.html">
 <link rel="import" href="../bower_components/app-layout/app-header-layout/app-header-layout.html">
 <link rel="import" href="../bower_components/app-layout/app-toolbar/app-toolbar.html">
-<link rel="import" href="../bower_components/app-layout/app-scroll-position/app-scroll-position.html">


Let's remove this old <app-scrollpos-control> too.

blasten · 2017-03-02T23:15:00Z

PTAL

keanulee · 2017-03-02T23:27:59Z

templates/pesto/src/recipe-app.html

@@ -118,21 +126,28 @@

          <!-- nav menu -->
          <paper-listbox
-              selected="{{_pageData.page}}"
+              selected="[[_pageData.page]]"


I understand why there needs to be a separate _selectedPage property, but why does this need to change to one-way bound _pageData.page with <a> links for navigation? This is an issue if a user clicks on the <paper-item> outside of the <a> (where the paper-item gets selected, but the link isn't clicked).

there's a bug in Polymer 2.0 that causes a notification to be missed. I will file the bug as soon as I can.

Let's add a comment with a TODO that we should change this back once the issue in Polymer2 is fixed.

Is there a tracking issue in polymer?

will file the bug asap.

done tracking issue is: Polymer/polymer#4405

frankiefu · 2017-03-02T23:55:53Z

templates/pesto/src/recipe-app.html

+      }
+      this._selectedPage = pageData.page;
+      if (map[pageData.page] != null) {
+        window.scrollTo(0, map[pageData.page]);


use Polymer.AppLayout.scroll({ top: scrollTop, behavior: 'silent' });

frankiefu · 2017-03-02T23:56:07Z

templates/publishing/src/blog-app.html

+        if (map[subroute.prefix] != null) {
+          window.scrollTo(0, map[subroute.prefix]);
+        } else if (this.isAttached) {
+          window.scrollTo(0, 0);


use Polymer.AppLayout.scroll({ top: scrollTop, behavior: 'silent' });

MeTaNoV · 2017-03-07T00:08:11Z

interesting... any other way to have a more flexible implementation?

frankiefu · 2017-03-07T19:31:53Z

@MeTaNoV We feel that there isn't a guaranteed way to make this element works reliably because to save the current scroll position it needs to do that before the page visibility changes. Ideally it should be handled inside iron-pages. We will look into providing a better solution in the future. For now you will need to save/restore scroll position in code. An example:

app-layout/templates/pesto/src/recipe-app.html

Lines 238 to 250 in 757d1fd

    
           _pageDataChanged: function(pageData, oldPageData) { 
        
             var map = this._scrollPositionMap; 
        
             if (oldPageData != null && oldPageData.page != null) { 
        
               map[oldPageData.page] = window.pageYOffset; 
        
             } 
        
             this._selectedPage = pageData.page; 
        
             if (map[pageData.page] != null) { 
        
               Polymer.AppLayout.scroll({ top: map[pageData.page], behavior: 'silent' }); 
        
             } else if (this.isAttached) { 
        
               Polymer.AppLayout.scroll({ top: 0, behavior: 'silent' }); 
        
             } 
        
           }

frankiefu · 2017-03-07T20:32:22Z

@blasten Just need to add the comment and otherwise LGTM

MeTaNoV · 2017-03-07T20:44:16Z

thx @frankiefu for your answer, I agree that it should be handled in any since there is not only iron-pages but others (including my own iron-swipeable-pages) that need to handle this. Do you think that a behaviour could be extracted to be used by the different elements or by extending a base iron-pages we could reuse the implementation? I am not yet familiar with 2.0 so I cannot tell directly...
@TimvdLippe might be probably interested also!

frankiefu · 2017-03-07T20:47:28Z

@MeTaNoV Yeah, a behavior makes a lot of sense. Good suggestion!

Remove app-scroll-position

4dd4304

googlebot added the cla: yes label Mar 1, 2017

blasten assigned keanulee and frankiefu Mar 1, 2017

frankiefu requested changes Mar 1, 2017

View reviewed changes

Address feedback

9b8af8b

Merge branch '2.0-preview' into 2.0-preview-remove-app-scrollpos

8498f14

keanulee suggested changes Mar 2, 2017

View reviewed changes

frankiefu requested changes Mar 3, 2017

View reviewed changes

Use Polymer.AppLayout.scroll

4fea0e2

Merge branch '2.0-preview' into 2.0-preview-remove-app-scrollpos

757d1fd

MeTaNoV mentioned this pull request Mar 7, 2017

[feature] Adding iron-scroll-pos-behavior to be used by any XXX-pages element PolymerElements/iron-scroll-target-behavior#31

Open

Add comment about issue in polymer 2.x

0f9b217

keanulee approved these changes Mar 10, 2017

View reviewed changes

frankiefu approved these changes Mar 13, 2017

View reviewed changes

frankiefu merged commit d1aa650 into 2.0-preview Mar 13, 2017

frankiefu deleted the 2.0-preview-remove-app-scrollpos branch March 13, 2017 03:30

web-padawan mentioned this pull request May 3, 2017

Is app-scrollpos-control deprecated in Polymer 2? #469

Open

HughxDev mentioned this pull request Jun 5, 2017

app-scroll-position doesn’t exist Polymer/polymer#4664

Closed

frankiefu mentioned this pull request Jun 6, 2017

Update info on multiple scrolling views from app/2.0/toolbox/app-layout.md Polymer/old-docs-site#2189

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove app-scroll-position #417

Remove app-scroll-position #417

blasten commented Mar 1, 2017

frankiefu left a comment

frankiefu Mar 1, 2017

blasten Mar 9, 2017

frankiefu Mar 1, 2017

blasten Mar 9, 2017

blasten commented Mar 2, 2017

keanulee Mar 2, 2017

blasten Mar 2, 2017 •

edited

Loading

frankiefu Mar 7, 2017

keanulee Mar 8, 2017

blasten Mar 9, 2017

blasten Mar 9, 2017

frankiefu Mar 2, 2017

frankiefu Mar 2, 2017

MeTaNoV commented Mar 7, 2017

frankiefu commented Mar 7, 2017

frankiefu commented Mar 7, 2017

MeTaNoV commented Mar 7, 2017

frankiefu commented Mar 7, 2017

Remove app-scroll-position #417

Remove app-scroll-position #417

Conversation

blasten commented Mar 1, 2017

frankiefu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blasten commented Mar 2, 2017

Choose a reason for hiding this comment

blasten Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MeTaNoV commented Mar 7, 2017

frankiefu commented Mar 7, 2017

frankiefu commented Mar 7, 2017

MeTaNoV commented Mar 7, 2017

frankiefu commented Mar 7, 2017

blasten Mar 2, 2017 •

edited

Loading