MM-14949 Correct scroll when new posts are loaded #2687
Changes from 3 commits
904e300
e3fa852
4b9e817
eb9fde6
deca543
e3c6be1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import PropTypes from 'prop-types'; | |
import React from 'react'; | ||
import AutoSizer from 'react-virtualized-auto-sizer'; | ||
import {DynamicSizeList} from 'react-window'; | ||
import {debounce} from 'mattermost-redux/actions/helpers'; | ||
|
||
import LoadingScreen from 'components/loading_screen.jsx'; | ||
|
||
|
@@ -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; | ||
|
||
const virtListStyles = { | ||
position: 'absolute', | ||
bottom: '0', | ||
maxHeight: '100%', | ||
}; | ||
|
||
const postListStyle = { | ||
padding: '14px 0px 7px', //21px of height difference from autosized list below at DynamicSizeList height change postListHeightChangeForPadding accordingly | ||
}; | ||
|
||
export default class PostList extends React.PureComponent { | ||
static propTypes = { | ||
|
||
|
@@ -101,6 +114,7 @@ export default class PostList extends React.PureComponent { | |
}; | ||
|
||
this.listRef = React.createRef(); | ||
this.postlistRef = React.createRef(); | ||
if (isMobile) { | ||
this.scrollStopAction = new DelayedAction(this.handleScrollStop); | ||
} | ||
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. One approach is to mock return value of 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 commentThe 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 |
||
if (this.postlistRef && this.postlistRef.current) { | ||
const postsAddedAtTop = this.state.postListIds.length !== prevState.postListIds.length && this.state.postListIds[0] === prevState.postListIds[0]; | ||
const channelHeaderAdded = this.state.atEnd !== prevState.atEnd && this.state.postListIds.length === prevState.postListIds.length; | ||
if (postsAddedAtTop || channelHeaderAdded) { | ||
const previousScrollTop = this.postlistRef.current.scrollTop; | ||
const previousScrollHeight = this.postlistRef.current.scrollHeight; | ||
|
||
return { | ||
previousScrollTop, | ||
previousScrollHeight, | ||
}; | ||
} | ||
} | ||
return null; | ||
} | ||
|
||
componentDidUpdate(prevProps, prevState, snapshot) { | ||
if (prevProps.channelLoading && !this.props.channelLoading) { | ||
this.loadPosts(this.props.channel.id, this.props.focusedPostId); | ||
} | ||
|
||
if (!this.postlistRef.current || !snapshot) { | ||
return; | ||
} | ||
|
||
const postlistScrollHeight = this.postlistRef.current.scrollHeight; | ||
const postsAddedAtTop = this.state.postListIds.length !== prevState.postListIds.length && this.state.postListIds[0] === prevState.postListIds[0]; | ||
const channelHeaderAdded = this.state.atEnd !== prevState.atEnd && this.state.postListIds.length === prevState.postListIds.length; | ||
if (postsAddedAtTop || channelHeaderAdded) { | ||
const scrollValue = snapshot.previousScrollTop + (postlistScrollHeight - snapshot.previousScrollHeight); | ||
if (scrollValue !== 0 && (scrollValue - snapshot.previousScrollTop) !== 0) { | ||
this.listRef.current.scrollTo(scrollValue, scrollValue - snapshot.previousScrollTop, !this.state.atEnd); | ||
} | ||
} | ||
} | ||
|
||
componentWillUnmount() { | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. I think you can write this as
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup true. Removed the arrow func |
||
this.loadMorePosts(); | ||
}); | ||
} else if (this.mounted) { | ||
this.setState({autoRetryEnable: false}); | ||
} | ||
|
@@ -317,8 +364,8 @@ export default class PostList extends React.PureComponent { | |
visibleStartIndex, | ||
visibleStopIndex, | ||
}) => { | ||
this.updateFloatingTimestamp(visibleStopIndex); | ||
this.checkBottom(visibleStartIndex); | ||
this.updateFloatingTimestamp(visibleStartIndex); | ||
this.checkBottom(visibleStopIndex); | ||
} | ||
|
||
initScrollToIndex = () => { | ||
|
@@ -443,7 +490,7 @@ export default class PostList extends React.PureComponent { | |
{({height, width}) => ( | ||
<DynamicSizeList | ||
ref={this.listRef} | ||
height={height} | ||
height={height - postListHeightChangeForPadding} | ||
width={width} | ||
className='post-list__dynamic' | ||
itemCount={this.state.postListIds.length} | ||
|
@@ -454,10 +501,11 @@ export default class PostList extends React.PureComponent { | |
onScroll={this.onScroll} | ||
onItemsRendered={this.onItemsRendered} | ||
initScrollToIndex={this.initScrollToIndex} | ||
onNewItemsMounted={this.onNewItemsMounted} | ||
canLoadMorePosts={this.canLoadMorePosts} | ||
skipResizeClass='col__reply' | ||
style={dynamicListStyle} | ||
innerRef={this.postlistRef} | ||
style={{...virtListStyles, ...dynamicListStyle}} | ||
innerListStyle={postListStyle} | ||
> | ||
{this.renderRow} | ||
</DynamicSizeList> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,6 +116,7 @@ describe('PostList', () => { | |
|
||
describe('initScrollToIndex', () => { | ||
test('should return index of start of new messages and call increasePostVisibility when all posts are unread', () => { | ||
baseProps.actions.increasePostVisibility.mockResolvedValue({moreToLoad: false}); | ||
const postListIds = []; | ||
for (let i = 0; i < 30; i++) { | ||
postListIds.push(`post${i}`); | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Might not need to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
const wrapper = shallow(<PostList {...baseProps}/>); | ||
wrapper.setState({atBottom: true}); | ||
wrapper.instance().onItemsRendered({visibleStartIndex: 4, visibleStopIndex: 1}); | ||
expect(wrapper.state('atBottom')).toEqual(false); | ||
}); | ||
}); | ||
}); |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 aspostListHeightChangeForPadding
is an integer assignment i wanted it to be at the top