cyclestreets / cyclescape

Cyclescape - cycle campaign group toolkit
https://www.cyclescape.org/
MIT License
33 stars 15 forks source link

New design migration: AJAX scroll mechanism within thread #1000

Closed mvl22 closed 2 years ago

mvl22 commented 2 years ago

The new design will have a better mechanism for loading messages, which is in line with modern interfaces like Messenger.

The page will show the unread messages onwards, and scrolling up will dynamically load earlier messages.

I would like to get as much of this mechanism of this working on the current site, to lower migration efforts. If necessary this can use a 'load earlier messages' button rather than detect a scroll up.

nikolai-b commented 2 years ago

I've put a very very rough implementation of this up on staging to check it is the direction you are after.

mvl22 commented 2 years ago

Yes, that's going in the right direction - thanks.

As you say it's a bit rough at the moment, e.g. there's an off-by-one bug I think as I get a blank message just after scroll-up, but do keep going.

I think we need to work on the assumption that it's likely that a few messages above the current will want to be seen more immediately, rather than having to wait for them, so they should probably be included in the initial load. Recent messages will often be scanned by a user to provide a quick reminder of the context of the so-far unread messages.

Also, I think to keep it performant, it might be worth considering how many messages the scroll-up should load at a time. I assume loading say 100 messages would be quite slow, and I think that's more than is necessary at once. We probably want about 20 at a time.

nikolai-b commented 2 years ago

I don't think loading 100 should be that slow. I checked some of the largest threads, e.g. /threads/500 which has 1108 messages on the copy I am testing it on, and it loads in under 800ms. I've also increased the initial load so we load all the new messages and 10 seen messages. We can play with the numbers but it should be ready for another test on staging.

nikolai-b commented 2 years ago

@mvl22 let me know when you have had a look and we can get this merged. This won't work on IE 11 without a polyfill, which is easy enough to add, just to check we do want to support IE 11 even though Microsoft will stop supporting it on 2022-06-15

mvl22 commented 2 years ago

Thanks - am trying this now. /threads/832 is a good one to try on as it's so long!

Yes, please include the polyfill. I doubt we have many IE11 users, but it's sensible not to cut them off in advance of the new design migration.

mvl22 commented 2 years ago

Yep, this is basically working fine.

Is this supposed to load everything before, or just say 20 at a time? I noticed on /threads/832 it's a very long wait because it's loading hundreds, and I ended up pressing refresh as I assumed the process had crashed. I think progressively at a time, as per FB Messenger, would avoid that problem.

Could we avoid having two spinners - just one at the start is enough:

Screenshot 2022-01-05 at 17 02 21

nikolai-b commented 2 years ago

I've changed it to progressive loading (40 at a time) and cut the spinners down to one. I've also added the polyfill. I'll deploy soon unless I hear otherwise.

mvl22 commented 2 years ago

Thanks - looks good to go.

nikolai-b commented 2 years ago

Done in 34afd3ebb973792a7b6e7e9f9dadeadd8d605af6 and commits around it.

mvl22 commented 2 years ago

Well done, thanks for your work on this.