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

WIP: Infinite Scroll #941

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@ssrihari

ssrihari commented Mar 13, 2018

Summary

  • When at top (<= 500 scroll), load more posts
  • Debounce loading more posts, so it isn't triggered multiple times on scroll
  • Use a state to mark when more posts are loaded, so it can be triggered again.

Ticket Link

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit and component tests passed
  • Added or updated unit tests (required for all new features)
  • Has server changes (please link)
  • Has redux changes (please link)
  • Has UI changes
  • Includes text changes and localization file (.../i18n/en.json) updates
  • Touches critical sections of the codebase (auth, posting, etc.)
@koxen

This comment has been minimized.

Contributor

koxen commented Mar 13, 2018

Further discussion on infiniscrool problem: https://pre-release.mattermost.com/core/pl/iqp8uxu97frfixhb4s3f1oa6ec

@koxen

This comment has been minimized.

Contributor

koxen commented Mar 13, 2018

chrome_2018-03-13_16-45-13

@amyblais

This comment has been minimized.

Member

amyblais commented Mar 13, 2018

Hi @ssrihari,

Thank you for this contribution!

I tested on Mac and the scrolling initially works well, but I saw two issues:

  1. Sometimes when I switch between channels that I've scrolled up in it takes me somewhere to the middle of a channel (instead of to the bottom of a channel as expected) - this happens randomly and not every time so there are no clear repro steps.
  2. When I stay in a channel for some minutes without doing anything, the loader appears to get stuck (or at least there is the "Loading more messages..." indicator so I assume there are still more messages to load).

I'll also test on iOS and see if I have any additional feedback.

@jasonblais

This comment has been minimized.

Member

jasonblais commented Mar 13, 2018

@koxen When did you see the JavaScript error that you posted?

@koxen

This comment has been minimized.

Contributor

koxen commented Mar 13, 2018

@jasonblais when scrooling around and then switching channels fast

@amyblais

This comment has been minimized.

Member

amyblais commented Mar 13, 2018

Hi @ssrihari,

For iOS I saw similar issues as with Mac:

  1. Same as the one I described in my previous comment in point 1.
  2. When I stay in a channel for some minutes without doing anything, the loader doesn't get stuck but it slows down a little and the screen might become blank (white) when I start scrolling and/or switching channels after the short break.
@ssrihari

This comment has been minimized.

ssrihari commented Mar 14, 2018

Thanks for trying this out, @amyblais! I'll look into these soon.

@lindy65 lindy65 removed their assignment Mar 14, 2018

@lindy65

Once @ssrihari has taken a look at the issues amyblais mentioned above, I'll test on Windows & Android.

Meant to comment and not "review"

@lindy65 lindy65 self-assigned this Mar 14, 2018

@ssrihari

This comment has been minimized.

ssrihari commented Mar 19, 2018

@amyblais

When I stay in a channel for some minutes without doing anything, the loader appears to get stuck

This should be fixed in 79cf3c2. Is there a way to have the spinmint instance rebuilt on that to verify the fix?

Sometimes when I switch between channels that I've scrolled up in it takes me somewhere to the middle of a channel

Interestingly, I see this in the spinmint instance, but not locally.

I'm unable to reproduce the other issues, but I'll keep trying. Also, it's possible that the above fix fixed some other fixes as a side-effect.

@ssrihari

This comment has been minimized.

ssrihari commented Mar 19, 2018

Just tested on the new spinmint instance.

On switching channels, it scrolls up to the point where there are new messages (I believe this is expected). If the new messages indicator is dismissed (which I was able to do by sending a test message in the channel), then switching to the channel scrolls to the bottom as expected.

Sometimes, when the new messages are at the scroll-top, we load more messages on top of the indicator. But, only sometimes, this new messages indicator also disappears when that happens, and re-appears on switch back and forth to the channel. I don't know if that issue is related to this PR, though.

@esethna esethna self-assigned this Mar 19, 2018

@amyblais

This comment has been minimized.

Member

amyblais commented Mar 19, 2018

Hi @ssrihari,

I re-tested this and as you mentioned the loader doesn't get stuck anymore in any situation (Mac) and the screen doesn't get blank (iOS), awesome!

For the other issue ("Sometimes when I switch between channels that I've scrolled up in it takes me somewhere to the middle of a channel") it still reproduces as you mentioned - I'm thinking maybe it's best to have a Dev's or second PM's opinion on what is expected - I'll ping a colleague to take a look.

@esethna

This comment has been minimized.

Member

esethna commented Mar 22, 2018

Hi @ssrihari, this PR is great! Thanks for the initial reviews @amyblais. A few comments after testing:

  1. Looks like there is a bug on tall monitors. Observed the channel is frozen in this position after switching to a channel with lots of messages:

image

  1. I tested on a "Slow 3G" connection and observed some unexpected behavior, see vid:

slow_connection

a) As in the vid using a slow connection, if I switch to a channel and start scrolling up immediately, I see the large "Loading..." indicator. This loading animation should only be seen when channel content is loading for the first time after channel switching. Once the channel content loads and displays, I would only expect to see the "Loading more messages..." text at the top of the screen if I begin scrolling. Note that if I wait 10-20 seconds to start scrolling after channel switching the behavior is as expected.

image

b) As in the vid using a slow connection, I can reproduce the "loading more messages..." getting stuck and not loading any messages above a certain place. You can repro by setting your connection to "Slow 3G" in the console, switching to Town Square and immediately scrolling up as far as you can. It may be related to this JS error I received around the same time:

image

  1. Looks like there is still an issue with the scroll position when there are new messages in the channel (see vid). You can see that when I switch to the channel, I see no "New Messages" indicator. If I scroll up, the new messages indicator appears below the "Loading more messages..." text, as I scroll up the new messages indicator moves up as well. I would expect that when I switch to the channel, I am scrolled to a position where the new messages indicator is at the last message I read, and I only have to scroll down to read all new messages. This may involve adding a similar "Loading more messages..." text at the bottom of the screen if there are a lot of new messages and the user is scrolling down.

new messages

  1. As mentioned on pre-release, our UX team is reviewing this PR as well, they may have some additional feedback.

Really great work on this, super excited to get this in!

@ssrihari

This comment has been minimized.

ssrihari commented Apr 2, 2018

Just as an FYI, I haven't forgotten about this :) . I've been both unwell, and caught in a few personal things all of last week. I'll get to the issues this week.

ssrihari added some commits Mar 13, 2018

Add infinite scroll
- When at top (<= 500 scroll), load more posts
- Debounce loading more posts, so it isn't triggered multiple times
  on scroll
- Use a state to mark when more posts are loaded, so it can be
  triggered again.
Fix intermittent stalling of infinite scroll.
- Changing channels and scrolling up 2 pages would stall scrolling.
@esethna

This comment has been minimized.

Member

esethna commented Apr 3, 2018

Thanks @ssrihari! Our UX team has had a chance to review as well. A few more thoughts:

  1. Can we try increasing the number of posts loaded at a time? Comparing to other products it feels like we are loading fewer posts with each fetch, and that means the user is hitting the top of the channel and waiting more often.

  2. The static text "Loading more messages..." can sometimes feel frozen on slow connections, even though the messages are fetching in the background. A loading animation would provide more feedback. Our UX team recommended the following animation from our current set, can we please replace "Loading more messages..." with the following animation:

ezgif-1-42003a345a

Adding @asaadmahmood to help point you in the right direction regarding the animation.

@asaadmahmood

This comment has been minimized.

Contributor

asaadmahmood commented Apr 5, 2018

That animation can be found in:
root.html

<div
    class='loading-screen'
    style='position: relative'
>
    <div class='loading__content'>
        <div class='round round-1'></div>
        <div class='round round-2'></div>
        <div class='round round-3'></div>
    </div>
</div>

However, the animation is supposed to appear in the middle, and it is a bit big, so you would have to modify the css a bit by adding a modifier class to it. if you are not sure about that, then you can ping me, and I can do that once you've included that as a loading animation.

@esethna

This comment has been minimized.

Member

esethna commented Apr 18, 2018

@ssrihari please let us know if you need help with the above, looking forward to getting this in

@esethna esethna added this to the v4.11.0 milestone Apr 25, 2018

@esethna

This comment has been minimized.

Member

esethna commented May 2, 2018

@ssrihari just checking if there's anything we can help with to push this PR across the finish line?

@sitaktif

This comment has been minimized.

sitaktif commented May 9, 2018

Can we try increasing the number of posts loaded at a time?

Could we even increase the number of post we load at each iteration? e.g. double it (or x 1.5) up to a max? It is usually fair to assume that a user that scrolls once may want recent history, but then when that user that keeps scrolling more and more, they want e.g. last day's history (typical use case is if someone from a different timezone is catching up with the team in the morning).

@vingt-2

This comment has been minimized.

vingt-2 commented May 21, 2018

Any news on this?
Our team would love to have this in the next version of Mattermost :).

@cpanato

This comment has been minimized.

Member

cpanato commented May 23, 2018

Removed the test server.

@mattermost mattermost deleted a comment from mattermod May 23, 2018

@mattermost mattermost deleted a comment from mattermod May 23, 2018

@esethna esethna removed this from the v4.11.0 milestone May 25, 2018

@esethna

This comment has been minimized.

Member

esethna commented May 25, 2018

Aiming for v5.1 (July) for this.

@esethna esethna added this to the v5.1.0 milestone May 25, 2018

@vingt-2

This comment has been minimized.

vingt-2 commented May 26, 2018

Great news, can't wait. Thanks !

@esethna esethna modified the milestones: v5.1.0, v5.2.0 Jun 20, 2018

@esethna

This comment has been minimized.

Member

esethna commented Jun 20, 2018

Moving this to the v5.2.0 milestone as we are going to focus on a number of scroll improvements during the v5.1 release and then circle back to this PR once those changes are made.

@esethna esethna changed the title from Add infinite scroll to WIP: Infinite Scroll Jun 20, 2018

@amyblais amyblais removed this from the v5.2.0 milestone Jul 31, 2018

@lindy65 lindy65 removed their assignment Sep 15, 2018

@esethna esethna closed this Nov 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment