Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(infinite): add scroll in opposite direction #8099

Merged
merged 9 commits into from
Mar 15, 2017

Conversation

Manduro
Copy link
Contributor

@Manduro Manduro commented Sep 16, 2016

Short description of what this resolves:

This adds support for infinite scrolling towards the top of a view.

Changes proposed in this pull request:

  • Adds a position option to the InfiniteScroll component.
  • Calculates distance to top or bottom accordingly.
  • Maintains scroll position when adding content to the top.
  • Scrolls the page down on load when using position top.
  • Adds scrollDownOnLoad input to Content.

Ionic Version: 2.x

Fixes: #6696 #9237

@theromis
Copy link

theromis commented Oct 5, 2016

somebody will react to this pull request?

@Manduro
Copy link
Contributor Author

Manduro commented Oct 5, 2016

I've been using it and it works great for me. You do need to hook into the right viewcycle events to save the current and set the new scroll position or the browser will jump up with the new content. This implementation will differ per situation and should not be integrated (I think), but some documentation about it might be good.

@theromis
Copy link

theromis commented Oct 5, 2016

@jgw96 could you check this great pull request?

@manucorporat
Copy link
Contributor

manucorporat commented Oct 5, 2016

@Manduro thanks for the PR and yes we I will look into it, but right now ionic 2 is feature freeze until 2.0 final.

The whole team is focusing efforts to fix as much bugs as possible.
This PR looks good and small, it has a lot of chances to be accepted but I don't want to add new code in this part of the development.

does it make sense?

@Manduro
Copy link
Contributor Author

Manduro commented Oct 6, 2016

Sure, makes a lot of sense!

@jpbullalayao
Copy link

Hi @Manduro, thanks for this PR! Until Ionic merges this in, how can we use this in our own projects? Do we need to go into the node_modules and modify the JS files to reflect exactly your changes? Or is there a way to maybe install this file into our projects using the command line?

I have a chat feature in our app and would love to use this :)

@Manduro
Copy link
Contributor Author

Manduro commented Oct 24, 2016

@jpbullalayao I'm not sure what's the proper way to do that. Maybe someone from the Ionic team can chime in?

Editing inside your node_modules is possible, but not really advised as the changes you make are not 'saved'.

@jourdan-jobox
Copy link

Hi @Manduro, I'm using this code for the chat feature in my app; I was wondering how you implemented your HTML/CSS in order to handle the UI for this?

As you said, we need to perhaps set the new scroll position in order to prevent the browser from jumping with the new content. How did you implement the Typescript/JS for this in your situation?

@Manduro
Copy link
Contributor Author

Manduro commented Oct 27, 2016

Sure! This is a simple implementation. Key is to save the scroll position before adding new items to the list, and setting this same scroll position just after the new items are added to the dom. I'm definitely open to improvements, especially the part with the setInterval ;).

Code removed as it's incompatible and outdated with the current Ionic version.

@jourdan-jobox
Copy link

@Manduro Thanks so much!! This is helping a lot, I'll be happy to provide any feedback I might have after fully implementing this UI part.

Another question I have. What's MessageComponent? Is that another TS file that you created (I see that <messages> is also an HTML tag, so I assume those are related)? Would you mind sharing the code skeleton of that as well for us to understand how the subscription to the MessageComponent works?

@Manduro
Copy link
Contributor Author

Manduro commented Oct 28, 2016

My MessageComponent just a custom component of mine. You could also use the <ion-item>/Item component from Ionic.

@Jellevanlith35
Copy link

Does anyone know when Ionic will implement this feature with their version?

@pdrosos
Copy link

pdrosos commented Nov 18, 2016

I am also following this one with great interest. I need it for a chat in my app very soon.
@manucorporat it would be great if this one can make it in the final release, will make many people happy! Thanks!

@jourdan-jobox
Copy link

This would be great for me as well for RC5/Final Release!

@Manduro Manduro reopened this Jan 23, 2017
@Manduro
Copy link
Contributor Author

Manduro commented Jan 23, 2017

Rebased on current master!

@theromis
Copy link

theromis commented Feb 1, 2017

@Manduro Is it possible to see this PR in the ionic milestones?

@theromis
Copy link

theromis commented Feb 1, 2017

Thank you @Manduro

@brandyscarney @manucorporat Is it possible to see this PR in the ionic milestones?

@theromis
Copy link

theromis commented Feb 1, 2017

@jgw96 Pleeeeease accept this PR

@brandyscarney brandyscarney added this to the 2.1.0 milestone Feb 2, 2017
@brandyscarney
Copy link
Member

@theromis We'll take a look at it for the 2.1.0 release, thanks!

@jgw96 jgw96 self-assigned this Feb 10, 2017
@manucorporat manucorporat modified the milestones: 2.2.0, 2.3.0 Mar 4, 2017
@Manduro
Copy link
Contributor Author

Manduro commented Mar 6, 2017

@jgw96 I've added code to correctly maintain the scroll position when loading more at the top, give it a try!

I'm still looking for clues about where to add an automatic initial scroll to bottom. Added this as well!

@movrack
Copy link

movrack commented Mar 10, 2017

We also need this feature. Will it be in version 2.3.0 ?

@maitham
Copy link

maitham commented Mar 13, 2017

Can I use this now? or Do I have to wait for it to be merged to master?

@manucorporat
Copy link
Contributor

@Manduro the code looks pretty good, I will try to merge this in 2.3

*/
@Input()
get scrollDownOnLoad(): boolean {
return !!this._scrollDownOnLoad;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!! is probably not needed, right? since we are using isTrueProperty() in the setter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, this can be removed if you'd like. I just copied the getter and setter from @Input() fullscreen.

set position(val: string) {
if (val === POSITION_TOP || val === POSITION_BOTTOM) {
this._position = val;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add here a console.error() telling val was an invalid value, and the two possible valid values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

@@ -267,7 +286,23 @@ export class InfiniteScroll {
* to `enabled`.
*/
complete() {
if (this.state === STATE_LOADING) {
if (this._position === POSITION_TOP) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain a little bit the algorithm used here?
how many frames does it take? and why?

I think I get it, but still... you probably know better

Copy link
Contributor Author

@Manduro Manduro Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I'll add a few more comments as well.

New content is being added at the top, but the scrollTop position stays the same, which causes a scroll jump visually. This algorithm makes sure to prevent this.

(Frame 1)

  1. complete() is called, but the UI hasn't had time to update yet.
  2. Save the current content dimensions.
  3. Wait for the next frame using _dom.read, so the UI will be updated.

(Frame 2)

  1. Read the new content dimensions.
  2. Calculate the height difference and the new scroll position.
  3. Delay the scroll position change until other possible dom reads are done using _dom.write to be performant.

(Still frame 2, if I'm correct)

  1. Change the scroll position (= visually maintain the scroll position).
  2. Change the state to re-enable the InfiniteScroll. This should be after changing the scroll position, or it could cause the InfiniteScroll to be triggered again immediately.

(Frame 3)

@Manduro
Copy link
Contributor Author

Manduro commented Mar 15, 2017

@manucorporat Thanks! I've made some slight changes as requested. Haven't squashed to keep the history, so make sure to do that when merging.

Thanks for building this amazing framework :)

@manucorporat manucorporat merged commit 6918275 into ionic-team:master Mar 15, 2017
@Manduro Manduro deleted the patch-1 branch March 15, 2017 14:02
@manucorporat
Copy link
Contributor

manucorporat commented Mar 15, 2017

Merged! I also made a minor change:
e53bad1

Thanks a lot for the PR! awesome new feature!

manucorporat added a commit that referenced this pull request Mar 23, 2017
@AlanChauchet
Copy link

Hi @Manduro , I am trying to implement a chat application and I am using the infinite-scroll on top but unfortunatly I haven't been able to use your "Maintains scroll position when adding content to the top." feature. Once new data has loaded, it automatically scrolls back to the top, and then infinitely loading more and more.

@andrewvmail
Copy link

Im also having the same issue..

@adriangube
Copy link

@AlanChauchet I'm also trying to implement a chat application and i'm also having the same issue..

@stalniy
Copy link
Contributor

stalniy commented Aug 7, 2017

There are 2 issues in current implementation:

@stalniy
Copy link
Contributor

stalniy commented Aug 7, 2017

created a bug #12598 going to send PR shortly

@ghost
Copy link

ghost commented Aug 7, 2017

@stalniy InfiniteScroll.complete() should be called after the data is received from the server, so that's not the issue here.

The issue with the scroll position not updating properly is a known bug on WKWebView, which you can read more about here #11587

@stalniy
Copy link
Contributor

stalniy commented Aug 7, 2017

created a fix #12599

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.