-
Notifications
You must be signed in to change notification settings - Fork 2.7k
MM-14949 Correct scroll when new posts are loaded #2687
Conversation
* Check for state change in posts instead of props * Correct scroll when posts change at top or when header changes * use snapshot for taking height before updating postslist
@@ -116,10 +130,41 @@ export default class PostList extends React.PureComponent { | |||
window.addEventListener('resize', this.handleWindowResize); | |||
} | |||
|
|||
componentDidUpdate(prevProps) { | |||
getSnapshotBeforeUpdate(prevProps, prevState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was unable to add a test case for this @saturninoabril any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One approach is to mock return value of postlistRef
and watch for the snapshot at componentDidUpdate
whenever it's called. Maybe something like,
test('should return previous scroll position from getSnapshotBeforeUpdate', () => {
const wrapper = shallow(<PostList {...baseProps}/>);
const instance = wrapper.instance();
instance.componentDidUpdate = jest.fn();
instance.postlistRef = {current: {scrollTop: 10, scrollHeight: 100}};
wrapper.setState({atEnd: true});
expect(instance.componentDidUpdate).toHaveBeenCalledTimes(1);
expect(instance.componentDidUpdate.mock.calls[0][2]).toEqual({previousScrollTop: 10, previousScrollHeight: 100});
instance.postlistRef = {current: {scrollTop: 30, scrollHeight: 200}};
wrapper.setState({atEnd: false});
expect(instance.componentDidUpdate).toHaveBeenCalledTimes(2);
expect(instance.componentDidUpdate.mock.calls[1][2]).toEqual({previousScrollTop: 30, previousScrollHeight: 200});
// may add test combination of state changes necessary in computing getSnapshotBeforeUpdate
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added test cases but as discussed offline downgrading enzyme because of enzymejs/enzyme#2027
Hey @sudheerDev I think you need to rebase this branch in order for the CircleCI stuff runs |
@enahum I rebased it today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking comments
@@ -25,6 +26,18 @@ const OVERSCAN_COUNT_BACKWARD = window.OVERSCAN_COUNT_BACKWARD || 50; // Exposin | |||
const OVERSCAN_COUNT_FORWARD = window.OVERSCAN_COUNT_FORWARD || 100; // Exposing the value for PM to test will be removed soon | |||
const HEIGHT_TRIGGER_FOR_MORE_POSTS = window.HEIGHT_TRIGGER_FOR_MORE_POSTS || 1000; // Exposing the value for PM to test will be removed soon | |||
|
|||
const postListHeightChangeForPadding = 21; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment as of why this value is 21.
nvm, saw the comment below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe move this constant closer to postListStyle
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the postListStyle
closer as postListHeightChangeForPadding
is an integer assignment i wanted it to be at the top
components/post_view/post_list.jsx
Outdated
@@ -196,7 +241,9 @@ export default class PostList extends React.PureComponent { | |||
if (error) { | |||
if (this.autoRetriesCount < MAX_NUMBER_OF_AUTO_RETRIES) { | |||
this.autoRetriesCount++; | |||
this.loadMorePosts(); | |||
debounce(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can write this as
debounce(this.loadMorePosts);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup true. Removed the arrow func
@sudheerDev then I don't know why is it reporting a build failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 🎉, added comments but non-blocking.
@@ -25,6 +26,18 @@ const OVERSCAN_COUNT_BACKWARD = window.OVERSCAN_COUNT_BACKWARD || 50; // Exposin | |||
const OVERSCAN_COUNT_FORWARD = window.OVERSCAN_COUNT_FORWARD || 100; // Exposing the value for PM to test will be removed soon | |||
const HEIGHT_TRIGGER_FOR_MORE_POSTS = window.HEIGHT_TRIGGER_FOR_MORE_POSTS || 1000; // Exposing the value for PM to test will be removed soon | |||
|
|||
const postListHeightChangeForPadding = 21; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe move this constant closer to postListStyle
.
@@ -116,10 +130,41 @@ export default class PostList extends React.PureComponent { | |||
window.addEventListener('resize', this.handleWindowResize); | |||
} | |||
|
|||
componentDidUpdate(prevProps) { | |||
getSnapshotBeforeUpdate(prevProps, prevState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One approach is to mock return value of postlistRef
and watch for the snapshot at componentDidUpdate
whenever it's called. Maybe something like,
test('should return previous scroll position from getSnapshotBeforeUpdate', () => {
const wrapper = shallow(<PostList {...baseProps}/>);
const instance = wrapper.instance();
instance.componentDidUpdate = jest.fn();
instance.postlistRef = {current: {scrollTop: 10, scrollHeight: 100}};
wrapper.setState({atEnd: true});
expect(instance.componentDidUpdate).toHaveBeenCalledTimes(1);
expect(instance.componentDidUpdate.mock.calls[0][2]).toEqual({previousScrollTop: 10, previousScrollHeight: 100});
instance.postlistRef = {current: {scrollTop: 30, scrollHeight: 200}};
wrapper.setState({atEnd: false});
expect(instance.componentDidUpdate).toHaveBeenCalledTimes(2);
expect(instance.componentDidUpdate.mock.calls[1][2]).toEqual({previousScrollTop: 30, previousScrollHeight: 200});
// may add test combination of state changes necessary in computing getSnapshotBeforeUpdate
});
@@ -152,4 +153,13 @@ describe('PostList', () => { | |||
expect(wrapper.state('atEnd')).toEqual(true); | |||
}); | |||
}); | |||
|
|||
describe('onItemsRendered', () => { | |||
test('should set state atBottom false when visibleStopIndex is not 0', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might not need to be async
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -9,8 +9,17 @@ exports[`component/FileAttachment should match snapshot, after change from file | |||
href="#" | |||
onClick={[Function]} | |||
> | |||
<div | |||
className="post-image__load" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the snapshot. This was caused by the enzyme issue with 3.9.0 enzymejs/enzyme#2027
Summary
Main changes are part of mattermost/react-window#9
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
getSnapshotBeforeUpdate
and correct based on it.handleNewMeasurements
from previous logic to keep correcting the scroll positionWe 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 usehandleNewMeasurements
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