MM-15345 slicing posts in post_list #2732
Conversation
|
@sudheerDev idk what's going on but this is already up to date with the latest master right? |
Yup. I branched it off of master on 30th april after my new changes for virt list went in |
|
@sudheerDev Sorry, it's taken me a few days to get to this. Is this still needed with the PR to remove the virtualized post list on IE11? |
|
@hmhealey Ya, we need this for virt list as well. Normally virt list can mount upto 160 posts at a time. This keeps it down to 50 in channels speeding up the mount time. Other reason is that if we scroll really fast up in a heavily loaded channel we will be hitting top of mounted posts and then virt list start to mount next batch of new posts making the UX little odd. |
|
Isn't the virtualized list supposed to handle that though? It seems like we're doing the list's job for it there |
yes and no, you might have to go though the description here in the mattermost/react-window#9 a little. So, Normally items in virt list are absolute elements so they are measured and placed appropriately and we used to autocorrect position based on where the post is mounted(Above or below the fold) Autocorrection of posts had a drawback in certain cases such as elements which can collapse/expand manually by user because this used to contradict the way normally scroll correction happens. In a hypothetical situation of having 300 posts which already were mounted because of API calls. Any scroll changes by user would not require a scroll correction because we have the dimensions and we can work with that. If the same was a subsequent visit 160 posts are mounted and when you scroll up even though we can mount virt list does not have a way of correcting the posts so what i did is to write a logic to batch load virt list posts when user reaches to the top of mounted posts and there are more posts that can be mounted and correct the scroll similar to how we deal with more posts loading. Coming back to the two problems
Initial reason of adding this was for IE11 normal view(not virt list) but the selectors work just fine so we don't need specific logic for IE. This does solve the above two problems for virt list so i left it open. Now that i think about it as we don't need IE 11 support for slicing. i can push a prop with loader component which can be added to the top of mounted posts which solves the UX problem and probably pass another prop to determine the initial mount of posts like 60, 100 etc. I will check with eric or elias later with the mana estimate for that change and get back to this. |
|
FYI: We still need a minor change from here to not make API calls because the 1000px threshold can be reached by user with fast scrolling in virt list(we cannot mount posts at the same pace) and we will be calling loadPosts api with the top post from props which will be different and not intended because we might be midway through the posts. |
components/post_view/post_list.jsx
Outdated
| } else if (hasPostListIdsChanged) { | ||
| if (statePostListIds[0] !== propsPostListIds[0]) { | ||
| // Post added at bottom | ||
| const oldestPostId = PostList.getOldestVisiblePostId(statePostListIds); |
There was a problem hiding this comment.
This isn't going to work if the last post in the channel is a combined system message. The post will have an ID like user-activity-post1_post2, but getLastPostId will just return post2 then, so the findIndex call will return -1. But even if we change it so that it would get user-activity-post1_post2, that might not work because the previous post might be a join/leave post, so its ID would have changed to user-activity-post1_post2_post3.
I'm not sure how we could work around this since the combined system messages really make this difficult. Maybe we could do this slicing before the posts get combined by preparePostIdsForPostList. I've also thought about how we could try using timestamps here instead of post IDs since those never change, but we'd need more information about the posts to do that.
There was a problem hiding this comment.
@hmhealey Wouldn't the postID change if combined system has a new system post?. If new post is added the oldest postId from the system message changes so this should work IMO. If that is not the case then we can fix it by making sure we get the right postID from the system messages which would change if a new post is added or removed(Don't think there is a chance of this happening).
I am not in favour of the changes here. Problem with this PR as the unknowns which can cause hard to catch bugs with vague reports and this makes it really complicated for bi-directional scrolling.
I have a better solution for this. I will submit it first before working on other PR's
components/post_view/post_list.jsx
Outdated
| } | ||
|
|
||
| const topPostIdToBeAdded = PostList.getTopIdForPostList(state); |
There was a problem hiding this comment.
Does the virtualized list have anything like the ListHeaderComponent and ListFooterComponent props that RN's FlatList has? It would be really nice if we didn't have to add this onto the PostList every time
components/post_view/post_list.jsx
Outdated
|
|
||
| if (!oldestPostId) { | ||
| // loadMorePosts shouldn't be called if we don't already have posts | ||
| return; | ||
| } | ||
| if (this.state.postListIds.length - 1 !== this.props.postListIds.length) { |
There was a problem hiding this comment.
Is the - 1 to remove the extra row that contains things like the intro message?
There was a problem hiding this comment.
Could you add a comment explaining what this block does? It's not really obvious that it's showing more posts that were already loaded.
components/post_view/post_list.jsx
Outdated
|
|
||
| if (!oldestPostId) { | ||
| // loadMorePosts shouldn't be called if we don't already have posts | ||
| return; | ||
| } | ||
| if (this.state.postListIds.length - 1 !== this.props.postListIds.length) { | ||
| const increasedPostListIds = this.props.postListIds.slice(0, this.state.postListIds.length + 30); |
There was a problem hiding this comment.
Could this be done as part of increasePostVisibility action like in the mobile app? That would allow it to be tested as well.
There was a problem hiding this comment.
increasePostVisibility
In general is brittle because we just increment it with 30 when it is not the case most times. like system message which will be grouped etc
components/post_view/post_list.jsx
Outdated
| const propsPostListIds = props.postListIds; | ||
| let postListIds = props.postListIds || []; | ||
|
|
||
| if (propsPostListIds) { |
There was a problem hiding this comment.
I don't have any specific suggestions for how to improve this, but this block is really complicated and hard to understand.
* Add a prop for passing initRangeToRender * check for unread to pass if channel has already mounted
|
@hmhealey @enahum mattermost/react-window#12 along with the changes here solves the issue |
hmhealey
added
4: Reviews Complete
|
Leaving this to you to merge since I can't merge the react-window changes |
amyblais
added
Changelog/Not Needed
Using higher value for permalinks as we request 60 posts and here postListIds include timestamps as well so a buffer of 40Ids.
This PR assumes that we wont be making an API call/addition of new posts at the top without the state matching the props. i.e we add posts at the top only by the request made when reaching top.