PolymerElements / iron-list

Element for a virtual, "infinite" list
https://www.webcomponents.org/element/PolymerElements/iron-list
219 stars 131 forks source link

Consider not resetting scroll position on items change #460

Open MaKleSoft opened 7 years ago

MaKleSoft commented 7 years ago

Current any change to the items array causes the scroll position to be reset. In the _itemsChanged handler:

if (this._scrollTop > this._scrollOffset) {
  this._resetScrollPosition(0);
}

There may be some cases where this is appropriate but I would argue in the majority of cases (e.g. editing an item, deleting a single item etc.) , this is not the desired behavior. What is the reasoning behind doing this? Personally, I would like to simply remove those three lines and have users take care of resetting the scroll position manually when appropriate. Thoughts?

MaKleSoft commented 7 years ago

Bump. Can I at least get some feedback on whether this is has any chance to get fixed? Otherwise I'll have to fork, which I'd like to avoid.

keanulee commented 7 years ago

Looks like that code was added in #350, and I believe it's necessary to ensure the scroll region is filled with items. @blasten may have some more insight.

MaKleSoft commented 7 years ago

It's only necessary if the size of the items array has changed significantly which for the use cases I've pointed out is rarely the case. In any case, resetting all the way to 0 seems excessive. It would be nice if we could figure out what the new boundary is and then reset the scroller to that limit if and only if the current scroll position is beyond that boundary. Maybe that's what #350 was trying to achieve but apparently it doesn't work. I.e. editing only one item in the array will still reset the scroller to 0 every time.

ernsheong commented 7 years ago

Whoops I opened a new issue on the same matter: https://github.com/PolymerElements/iron-list/issues/476 Favoring this one.

You can use Polymer's splice method so that the "items.splices" portion of the code in _itemsChanged gets triggered instead.

However I find that I needed to call Polymer.flush myself to make the render happen. See https://github.com/PolymerElements/iron-list/issues/475

My use case is that I am using the Redux pattern, and replacing the items array instance with a new one.

MaKleSoft commented 7 years ago

@ernsheong The splice method is no longer supported in Polymer 2.0 afaik and we're not using hybrid mode.

ernsheong commented 7 years ago

@MaKleSoft According to 2.0 docs splice is still here https://www.polymer-project.org/2.0/docs/devguide/model-data#array-mutation, and I am calling it in 2.0 code, it works... just that you might have to call Polymer.flush on your own to force render

MaKleSoft commented 7 years ago

@ernsheong huh, no shit. I was sure I had read something about it being removed somewhere. Maybe it was in the preview and they added it back later. Thanks for pointing this out!

Anyway, I think my original point is still valid, especially if you are using the ImmutableData behavior.

ernsheong commented 7 years ago

Yea, my use case was using Redux pattern, replacing the entire array. Cheers!

daniele-orlando commented 6 years ago

I'm waiting for this feature too.

derhuebiii commented 5 years ago

Me, too.

I work around by temporary increasing $.iron-list.scrollOffset to a number higher than the $.iron-list.scrollTop and reset it after the data has been loaded, but not optimal. :)

ernsheong commented 5 years ago

Polymer 2/3 is in maintenance mode. If you are starting a new project, the recommended step is to start with https://github.com/Polymer/lit-element

It is unlikely that this component or any of its sister components will be getting any features anytime soon, if ever.