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

MM-14949 Correct scroll when new posts are loaded #2687

Merged
merged 6 commits into from Apr 30, 2019
Merged

MM-14949 Correct scroll when new posts are loaded #2687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
904e300
MM-14949 Correct scroll when new posts are loaded
sudheerDev Apr 22, 2019
e3fa852
update window hash
sudheerDev Apr 29, 2019
4b9e817
Remove test case for snapshot
sudheerDev Apr 29, 2019
eb9fde6
* Address review comments
sudheerDev Apr 30, 2019
deca543
Remove uneeded arrow func for debounce
sudheerDev Apr 30, 2019
e3c6be1
Update snapshot
sudheerDev Apr 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions components/file_attachment/__snapshots__/file_attachment.test.jsx.snap
View file
Open in desktop
Expand Up @@ -9,8 +9,17 @@ exports[`component/FileAttachment should match snapshot, after change from file
href="#"
onClick={[Function]}
>
<div
className="post-image__load"
Copy link
Contributor Author

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

<FileThumbnail
fileInfo={
Object {
"extension": "png",
"height": 400,
"id": 2,
"name": "test.png",
"size": 100,
"width": 600,
}
}
/>
</a>
<div
Expand Down
60 changes: 53 additions & 7 deletions components/post_view/post_list.jsx
View file
Open in desktop
Expand Up @@ -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';

Expand All @@ -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;
Copy link
Contributor

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

Copy link
Member

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.

Copy link
Contributor Author

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


const postListStyle = {
padding: '14px 0px 7px', //21px of height difference from autosized list compared to DynamicSizeList. If this is changed change the above variable postListHeightChangeForPadding accordingly
};

const virtListStyles = {
position: 'absolute',
bottom: '0',
maxHeight: '100%',
};

export default class PostList extends React.PureComponent {
static propTypes = {

Expand Down Expand Up @@ -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);
}
Expand All @@ -116,10 +130,41 @@ export default class PostList extends React.PureComponent {
window.addEventListener('resize', this.handleWindowResize);
}

componentDidUpdate(prevProps) {
getSnapshotBeforeUpdate(prevProps, prevState) {
Copy link
Contributor Author

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?

Copy link
Member

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
});

Copy link
Contributor Author

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

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() {
Expand Down Expand Up @@ -196,7 +241,7 @@ export default class PostList extends React.PureComponent {
if (error) {
if (this.autoRetriesCount < MAX_NUMBER_OF_AUTO_RETRIES) {
this.autoRetriesCount++;
this.loadMorePosts();
debounce(this.loadMorePosts());
} else if (this.mounted) {
this.setState({autoRetryEnable: false});
}
Expand Down Expand Up @@ -317,8 +362,8 @@ export default class PostList extends React.PureComponent {
visibleStartIndex,
visibleStopIndex,
}) => {
this.updateFloatingTimestamp(visibleStopIndex);
this.checkBottom(visibleStartIndex);
this.updateFloatingTimestamp(visibleStartIndex);
this.checkBottom(visibleStopIndex);
}

initScrollToIndex = () => {
Expand Down Expand Up @@ -443,7 +488,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}
Expand All @@ -454,10 +499,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>
Expand Down
40 changes: 40 additions & 0 deletions components/post_view/post_list.test.jsx
View file
Open in desktop
Expand Up @@ -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}`);
Expand Down Expand Up @@ -152,4 +153,43 @@ describe('PostList', () => {
expect(wrapper.state('atEnd')).toEqual(true);
});
});

describe('onItemsRendered', () => {
test('should set state atBottom false when visibleStopIndex is not 0', () => {
const wrapper = shallow(<PostList {...baseProps}/>);
wrapper.setState({atBottom: true});
wrapper.instance().onItemsRendered({visibleStartIndex: 4, visibleStopIndex: 1});
expect(wrapper.state('atBottom')).toEqual(false);
});
});
describe('Scroll correction logic on mount of posts at the top', () => {
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});

instance.postlistRef = {current: {scrollTop: 40, scrollHeight: 400}};
wrapper.setProps({postListIds: [
'post1',
'post2',
'post3',
'post4',
'post5',
DATE_LINE + 1551711600000,
]});

expect(instance.componentDidUpdate).toHaveBeenCalledTimes(3);
expect(instance.componentDidUpdate.mock.calls[2][2]).toEqual({previousScrollTop: 40, previousScrollHeight: 400});
});
});
});
28 changes: 18 additions & 10 deletions components/post_view/show_more/show_more.jsx
View file
Open in desktop
Expand Up @@ -41,6 +41,9 @@ export default class ShowMore extends React.PureComponent {

componentWillUnmount() {
window.removeEventListener('resize', this.handleResize);
if (this.overflowRef) {
window.cancelAnimationFrame(this.overflowRef);
}
}

toggleCollapse = (e) => {
Expand All @@ -53,18 +56,23 @@ export default class ShowMore extends React.PureComponent {
};

checkTextOverflow = () => {
const textContainer = this.refs.textContainer;
let isOverflow = false;

if (textContainer && textContainer.scrollHeight > this.props.maxHeight) {
isOverflow = true;
if (this.overflowRef) {
window.cancelAnimationFrame(this.overflowRef);
}
this.overflowRef = window.requestAnimationFrame(() => {
const textContainer = this.refs.textContainer;
let isOverflow = false;

if (isOverflow !== this.state.isOverflow) {
this.setState({
isOverflow,
});
}
if (textContainer && textContainer.scrollHeight > this.props.maxHeight) {
isOverflow = true;
}

if (isOverflow !== this.state.isOverflow) {
this.setState({
isOverflow,
});
}
});
};

handleResize = () => {
Expand Down
35 changes: 9 additions & 26 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Expand Up @@ -52,7 +52,7 @@
"react-select": "2.4.2",
"react-transition-group": "2.7.1",
"react-virtualized-auto-sizer": "^1.0.2",
"react-window": "github:mattermost/react-window#22cab3a8a0f4e3793ba778a990f1c0723b4c8129",
"react-window": "github:mattermost/react-window#cfb30dc14b065c10a93f3418006408d84c31a80f",
"rebound": "0.1.0",
"redux": "4.0.1",
"redux-batched-actions": "0.4.1",
Expand Down Expand Up @@ -84,7 +84,7 @@
"cross-env": "5.2.0",
"css-loader": "2.1.1",
"cypress": "3.2.0",
"enzyme": "3.9.0",
"enzyme": "3.8.0",
"enzyme-adapter-react-16": "1.11.2",
"enzyme-to-json": "3.3.5",
"eslint": "5.15.3",
Expand Down
14 changes: 2 additions & 12 deletions sass/layout/_post.scss
View file
Open in desktop
Expand Up @@ -284,7 +284,6 @@
}

.post-list-holder-by-time {
-webkit-overflow-scrolling: touch;
height: 100%;
width: 100%;
position: absolute;
Expand Down Expand Up @@ -413,14 +412,6 @@
padding: 15px;
}

.post-list__dynamic {
> div {
> div {
margin-bottom: 8px;
}
}
}

.post-create__container {
@include flex(0 0 auto);
width: 100%;
Expand Down Expand Up @@ -700,13 +691,12 @@

.post-list__table {
@include clearfix;
display: table;
height: 100%;
table-layout: fixed;
width: 100%;

.post-list__content {
display: table-cell;
height: 100%;
overflow: hidden;

.dropdown-menu {
&.bottom {
Expand Down