Skip to content
This repository has been archived by the owner on Mar 3, 2021. It is now read-only.

Change virtlist to based of off top values instead of bottom #9

Merged
merged 4 commits into from
Apr 30, 2019

Conversation

sudheerDev
Copy link

@sudheerDev sudheerDev commented Apr 22, 2019

  • Change to scroll correction logic

Previous logic used for correction of scroll is based on mount of every post in virt list which determines if post is added below the fold or above and corrects accordingly.
This has couple of downside mainly because it is hard to distinguish if the change of height is caused by user or the post itself.
i.e collpasing an attachment and change of post height because post header is removed automatically when more posts are loaded because it can be bunched.

These both contradict each other because when collapsing images we have to scroll up in the view. where as when a posts header changes because of new posts we have to keep it in place and move the newly added posts down.

The previous advantage of scroll correction is that it is easy to keep track of changes in heights of posts and keeping to the bottom of postlist is easy. Along with scroll pop correction caused by any random height change because it tries to auto correct itself

New logic works on two main principles for correction

  1. When new posts are loaded use getSnapshotBeforeUpdate and correct based on it.
  2. When new post arrives or scroll pops when at the bottom of channel use handleNewMeasurements from previous logic to keep correcting the scroll position

We still use handleNewMeasurements in a really specific scenario of keeping the scroll position locked when RHS with centre channel not at bottom. To achieve this we need to know the height changes of all the posts above the fold when RHS is opened as posts wrap to multiple lines causing content in center channel to be pushed. We use handleNewMeasurements and force correction when width of posts change i.e like resize of RHS opens.

https://mattermost.atlassian.net/browse/MM-13702
https://mattermost.atlassian.net/browse/MM-14947
https://mattermost.atlassian.net/browse/MM-14948

  • Absolute position of virt posts to relative positioning

To make changes to scroll correction logic as described above one of key changes was to change from absolute position to relative position because with absolute positioning we don't have a way of checking if all posts are in correct position to take a snapshot for scroll correction. Absolute position also causes weird issues as post overlap etc zoom flickers etc

This also gives us with an option to optimise different parts of the virt list such as measuring items, listening to changes of height changes by delaying them as the content flows with relative positioning and does not cause to overlap posts.

Virtualised lists are always done with absolute positioning and it makes sense only when using fixed height elements as it gives us an option to guess the positions of various elements without even mounting.

https://mattermost.atlassian.net/browse/MM-14831

  • Change from bottom to top positioning

It is hard to lock the scroll position when there are frequent messages coming into in virt list that is because if we have bottom positioning it tends to move to bottom and we have to intentionally correct to lock position. With changing it to top positioning we don't have to correct to lock but we have to correct to bottom when needed.

This also enables us to use the default browser behaviour of keeping scroll locked when content height below the fold changes so there is no extra logic of adding scroll corrections when an attachment is collapsed

https://mattermost.atlassian.net/browse/MM-14946
https://mattermost.atlassian.net/browse/MM-14949

 * Change virtlist to be relative position items instead of absolute items
 * Add empty div for out of range list items to fill space
 * Add an animation frame difference for positioning scrollbars
@sudheerDev sudheerDev force-pushed the fix/scrollLock branch 5 times, most recently from aa71079 to b6c0727 Compare April 29, 2019 16:37
* Add width 100% for outerRef container
* Change scrollToItem of 0 index to use scrollTo with element dimentions
* Add forceScrollCorrection if we need use mouted items scrollcorrections logic
  * used for correcting RHS based to keep scroll position in place
Copy link

@enahum enahum left a comment

Choose a reason for hiding this comment

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

one minor concern, but if that is not the case we can go along with it

src/DynamicSizeList.js Show resolved Hide resolved
@sudheerDev sudheerDev merged commit cfb30dc into feature/reverseScroll Apr 30, 2019
@sudheerDev sudheerDev deleted the fix/scrollLock branch April 30, 2019 07:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants