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

MM-14975 remove virt_list for IE11 #2747

Merged
merged 6 commits into from
May 9, 2019
Merged

MM-14975 remove virt_list for IE11 #2747

merged 6 commits into from
May 9, 2019

Conversation

sudheerDev
Copy link
Contributor

Summary

Main changes are adding back old post_list code for fallback to enable older browsers to work without virt_list.

Includes using new event emitter for propagating scroll changes.
Includes new lifecycle methods for scroll corrections.

Ticket Link

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

@sudheerDev sudheerDev requested a review from enahum May 6, 2019 21:56
@sudheerDev sudheerDev added this to the v5.12.0 milestone May 6, 2019
@sudheerDev sudheerDev requested a review from hmhealey May 6, 2019 21:58
@sudheerDev sudheerDev force-pushed the MM-14975 branch 2 times, most recently from 7b2654c to 5852bd0 Compare May 6, 2019 22:09
enahum
enahum suggested changes May 7, 2019
View reviewed changes
components/post_view/post_list_normal/post_list_normal.jsx Outdated
<div className='post-list__loading post-list__loading-search'>
<FormattedMessage
id='posts_view.maxLoaded'
defaultMessage='Looking for a specific message? Try searching for it'
Copy link
Contributor

Choose a reason for hiding this comment

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

@sudheerDev you need to add this string to the en.json file

Copy link
Member

Choose a reason for hiding this comment

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

We could consider just removing this logic. MAX_POST_VISIBILITY is 1000000, and there's no way you'll be able to load or render that many posts in the app anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya true. Weird on how we even came up with the number on the first place

hmhealey
hmhealey reviewed May 7, 2019
View reviewed changes
components/post_view/post_list_virtualized/post_list_virtualized.jsx Outdated
@@ -126,7 +126,7 @@ export default class PostList extends React.PureComponent {

window.addEventListener('resize', this.handleWindowResize);

EventEmitter.addListener('scroll_post_list_to_bottom', this.scrollToBottom);
EventEmitter.addListener(EventTypes.POST_LIST_SCROLL_CHANGE, this.scrollToBottom);
Copy link
Member

Choose a reason for hiding this comment

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

Do these need to be changed to use this.scrollChange?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Fixed it.

components/post_view/reaction_list/reaction_list.jsx
@@ -66,6 +68,12 @@ export default class ReactionList extends React.PureComponent {
}
}

componentDidUpdate(prevProps) {
if (this.props.reactions !== prevProps.reactions && disableVirtList()) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to check disableVirtList() here? If the virtualized post list doesn't respond to scrollPostList, then it's not really necessary. Then again, this does make it really clear that this can be removed if disableVirtList is. I'm 0/5 on this either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya it makes it obvious so i am inclined to leaving the way it is

@hmhealey hmhealey added the 2: Dev Review Requires review by a core commiter label May 7, 2019
@sudheerDev
Copy link
Contributor Author

@hmhealey Assign me for merging this PR. I will be making some changes for perf improvements today. so i want to hold this for another day to see if we can avoid this.

@enahum
Copy link
Contributor

enahum commented May 8, 2019

@sudheerDev perf improvements on IE11? Do you really think is worth the time? Maybe we should move on

@sudheerDev
Copy link
Contributor Author

sudheerDev commented May 8, 2019 •
edited
Loading

@sudheerDev perf improvements on IE11? Do you really think is worth the time? Maybe we should move on

i am not going to spend time on it but just want to get a feeling of how this #2757 and the related react-window PR change performs on IE11.

If we can avoid this PR with just adding couple of IE issues because of virt list i think it is an acceptable trade. One reason being we still have to make changes to this logic for bi-directional scrolling to support IE.

Related PR's felt really good for me on safari.

@enahum
Copy link
Contributor

enahum commented May 8, 2019

One reason being we still have to make changes to this logic for bi-directional scrolling to support IE

Not sure, to me disabling virtualized post list should also disable bi-directional scroll.

@hmhealey
Copy link
Member

hmhealey commented May 8, 2019

I think it's fine not to support bi-directional scrolling on IE11 if it ends up being very difficult. We'd need to maintain the old version of permalink view for IE, but it might be easier to do that than to implement two different versions of bi-directional scrolling

@hmhealey hmhealey added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels May 8, 2019
@sudheerDev
Copy link
Contributor Author

sudheerDev commented May 8, 2019 •
edited
Loading

@hmhealey @enahum This open PR #2274 simplifies the old post_list logic so i can still use that with changes for virt list and i think it might work just fine, might even end up with a lot cleaner code. I am not a fan of older post list logic. Most of it is added because of channel sync issue with routing and this change https://github.com/mattermost/mattermost-webapp/pull/2274/files#diff-0ff7df265ab43f5f102437c059a2407eR172 here avoids it

@sudheerDev sudheerDev merged commit 64fdedc into mattermost:master May 9, 2019
@sudheerDev sudheerDev deleted the MM-14975 branch May 9, 2019 16:25
migbot added a commit that referenced this pull request May 10, 2019
@migbot
Revert "MM-14975 remove virt_list for IE11 (#2747)"
131188b 
This reverts commit 64fdedc.
migbot added a commit that referenced this pull request May 10, 2019
@migbot
Revert "MM-14975 remove virt_list for IE11 (#2747)" (#2774)
496360e 
This reverts commit 64fdedc.
sudheerDev added a commit that referenced this pull request May 13, 2019
@sudheerDev
Revert "Revert "MM-14975 remove virt_list for IE11 (#2747)" (#2774)"
3f2006e 
This reverts commit 496360e.
migbot pushed a commit that referenced this pull request May 13, 2019
@sudheerDev @migbot
Revert "Revert "MM-14975 remove virt_list for IE11 (#2747)"" (#2779)
3c57d59 
* Revert "Revert "MM-14975 remove virt_list for IE11 (#2747)" (#2774)"

This reverts commit 496360e.

* MM-15516  Web: New messages separator is missing

* Add lastViewedAt from state to preparePostIdsForPostList for New messages separator
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Changelog/Done Required changelog entry has been written and removed Changelog/Not Needed Does not require a changelog entry labels May 17, 2019
@lindy65 lindy65 added Tests/Done Release tests have been written and removed 4: Reviews Complete All reviewers have approved the pull request labels May 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hmhealey hmhealey hmhealey approved these changes

@enahum enahum enahum approved these changes

Assignees
No one assigned
Labels
Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
Milestone
v5.12.0
Development

Successfully merging this pull request may close these issues.
5 participants
@sudheerDev @enahum @hmhealey @lindy65 @amyblais