MM-41271 : Don't render sidebar channels when collapsed #9732

M-ZubairAhmed · 2022-02-01T15:00:03Z

Summary

Collapsed menu in sidebar are removed from the DOM to improve performance.

Ticket Link

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

Related Pull Requests

None

Screenshots

Screen.Recording.2022-02-03.at.5.59.06.pm.mov

Release Note

Removes collapsed sidebar menu from the dom on sidebar collapse and vise versa on sidebar expand

components/sidebar/sidebar_channel/sidebar_channel.tsx

hmhealey · 2022-02-01T23:48:39Z

components/sidebar/sidebar_channel/sidebar_channel.tsx

@@ -215,6 +248,10 @@ export default class SidebarChannel extends React.PureComponent<Props, State> {
 );
 }

+ if (!this.state.show) {


Could you move this up closer to the top of the render? I know it won't make much of a difference to skip the whole setup of wrappedComponent, but it's nice to return early when rendering null doesn't depend on any of the other work done by this function.

There were few UI gliches when we werent rendering the draggable components so i moved this check above https://github.com/mattermost/mattermost-webapp/pull/9732/files#diff-4c9f89f66f73c204771dc4e66a58fb0c67b346703ea3753e8c3337a99e9f9540R175
I have updated the screen grab to show this

Okay, that should be fine. I assume the heaviest part of those extra channels in the sidebar is the actual channelComponent, so that should still be a performance improvement. I will have to update my queries I've been asking people to run since they'll still count those empty draggable items

esethna · 2022-02-02T14:35:17Z

@M-ZubairAhmed @hmhealey Will this have a negative perceived performance impact when a user expands a channel category?

hmhealey · 2022-02-02T19:07:45Z

No, it shouldn't. We expect this to be a decent performance improvement for anyone with a lot of channels in their sidebar who keeps any categories collapsed though.

components/sidebar/sidebar_channel/sidebar_channel.tsx

hmhealey · 2022-02-04T15:59:54Z

components/sidebar/sidebar_channel/sidebar_channel.tsx

+
+ // On fully expanded
+ if (event.propertyName === 'height' && !this.isCollapsed(this.props)) {
+ this.setState({show: true});


This actually needs to be set at the start of transition so that the channel is shown as it expands. Right now, it renders nothing while the channel is expanding until it just pops in

Screen.Recording.2022-02-04.at.11.00.21.AM.mov

Yes i fixed it. I changed my approach to using the css 3 animation. This was done because

browser can optimize for animation steps

animation start and end event listeners are typed correctly in react types, where as transition start doesn't exist in react dom types.

test update

M-ZubairAhmed · 2022-02-06T21:42:28Z

sass/layout/_sidebar-left.scss

- transition-timing-function: ease, ease, ease, ease-in, step-start;
+ &.expanded {
+ animation-direction: normal;
+ animation-duration: 0.4s;


If you notice transition duration and animation duration are different: 0.15 and 0.18 respectively. Take a look at the fps comparison with both being dissimilar and being same.

With dissimilar duration

With same duration

The one with the same duration shows few frames were dropped at the starting of the menu collapsing. This reduced in frame rate resulted in choppy transition. On the other hand the one with different duration shows no frame drop resulting in near 60 fps. Height collapsing is a layout shift operation which requires more effort than opacity since browser has to paint change in height in every step. Change in duration gives browser enough room to perform the transition. (performance profiles available in g drive)

jespino

LGTM

jgilliam17 · 2022-02-09T21:40:24Z

@M-ZubairAhmed
Tested manually - no issues. Verified sidebar collapse and expand is working as expected. Also, new messages in collapsed categories are received as expected, as is the sorting to Unreads from collapsed categories.
E2E report has a failure that seems related to this PR MM-T2003 Can you please take a look? Thanks

M-ZubairAhmed · 2022-02-10T10:33:21Z

@jgilliam17 it seems like that is the old report, i have fixed all the related e2es for this pr, here is the latest report https://automation-dashboard.vercel.app/cycles/1455f4cf-3731-4819-af4d-4e986cfca0a4

jgilliam17 · 2022-02-10T14:09:15Z

@M-ZubairAhmed Thanks for the latest report. Yes, everything looks good, no PR related failures. Good to merge.

mm-cloud-bot · 2022-02-10T14:09:41Z

Test server destroyed

amyblais · 2022-02-11T17:03:34Z

/cherry-pick release-6.3

mattermod · 2022-02-11T17:03:36Z

Cherry pick is scheduled.

) Collapsed menu in sidebar are removed from the DOM to improve performance. (cherry picked from commit 6333537)

Collapsed menu in sidebar are removed from the DOM to improve performance. (cherry picked from commit 6333537) Co-authored-by: Md_ZubairAhmed <[email protected]>

… (#9799)" This reverts commit 7846744.

… (#9799)" (#9850) This reverts commit 7846744.

amyblais · 2022-03-07T14:32:56Z

/cherry-pick release-6.3

mattermod · 2022-03-07T14:32:58Z

Cherry pick is scheduled.

) Collapsed menu in sidebar are removed from the DOM to improve performance. (cherry picked from commit 6333537)

Automatic Merge

M-ZubairAhmed added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Feb 1, 2022

M-ZubairAhmed requested a review from hmhealey February 1, 2022 15:00

mm-cloud-bot added the release-note label Feb 1, 2022

hmhealey suggested changes Feb 1, 2022

View reviewed changes

hmhealey requested a review from devinbinnie February 1, 2022 23:48

jespino reviewed Feb 3, 2022

View reviewed changes

components/sidebar/sidebar_channel/sidebar_channel.tsx Outdated Show resolved Hide resolved

M-ZubairAhmed force-pushed the MM-41271 branch 2 times, most recently from a6e77c3 to 60b1d06 Compare February 3, 2022 11:56

M-ZubairAhmed requested review from jespino and hmhealey February 3, 2022 11:58

M-ZubairAhmed force-pushed the MM-41271 branch 2 times, most recently from 08b318a to 3cf656e Compare February 3, 2022 12:55

hmhealey added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Feb 4, 2022

hmhealey suggested changes Feb 4, 2022

View reviewed changes

Implementation with css transition

ca84b7d

test update

M-ZubairAhmed force-pushed the MM-41271 branch 3 times, most recently from 165db1f to 1d6ce8f Compare February 6, 2022 20:06

changed implementation to use css 3 animations

6828e2e

M-ZubairAhmed force-pushed the MM-41271 branch from 1d6ce8f to 6828e2e Compare February 6, 2022 20:54

mattermost deleted a comment from mm-cloud-bot Feb 6, 2022

M-ZubairAhmed requested a review from hmhealey February 6, 2022 21:20

M-ZubairAhmed commented Feb 6, 2022

View reviewed changes

M-ZubairAhmed added the 1: UX Review Requires review by a UX Designer label Feb 7, 2022

jespino approved these changes Feb 7, 2022

View reviewed changes

jgilliam17 approved these changes Feb 10, 2022

View reviewed changes

jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Feb 10, 2022

jgilliam17 assigned M-ZubairAhmed Feb 10, 2022

M-ZubairAhmed merged commit 6333537 into master Feb 10, 2022

M-ZubairAhmed deleted the MM-41271 branch February 10, 2022 14:10

M-ZubairAhmed removed their assignment Feb 10, 2022

mattermost-build mentioned this pull request Feb 11, 2022

Automated cherry pick of #9732 #9799

Merged

mattermod added the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Feb 11, 2022

amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Feb 17, 2022

amyblais added a commit that referenced this pull request Feb 18, 2022

Revert "MM-41271 : Don't render sidebar channels when collapsed (#9732)…

bcaeb64

… (#9799)" This reverts commit 7846744.

hmhealey pushed a commit that referenced this pull request Feb 18, 2022

Revert "MM-41271 : Don't render sidebar channels when collapsed (#9732)…

9a53bbd

… (#9799)" (#9850) This reverts commit 7846744.

amyblais removed the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Mar 1, 2022

mattermost-build mentioned this pull request Mar 7, 2022

Automated cherry pick of #9732 #9957

Merged

mattermod added the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Mar 7, 2022

mattermod pushed a commit that referenced this pull request Mar 7, 2022

MM-41271 : Don't render sidebar channels when collapsed (#9732) (#9957)

435aa5c

Automatic Merge

M-ZubairAhmed mentioned this pull request Dec 13, 2022

MM-48895 : Remove tab index on collapsed channel link in sidebar #11794

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-41271 : Don't render sidebar channels when collapsed #9732

MM-41271 : Don't render sidebar channels when collapsed #9732

M-ZubairAhmed commented Feb 1, 2022 •

edited

Loading

hmhealey Feb 1, 2022

M-ZubairAhmed Feb 3, 2022 •

edited

Loading

hmhealey Feb 4, 2022

esethna commented Feb 2, 2022

hmhealey commented Feb 2, 2022

hmhealey Feb 4, 2022

M-ZubairAhmed Feb 6, 2022

M-ZubairAhmed Feb 6, 2022 •

edited

Loading

jespino left a comment

jgilliam17 commented Feb 9, 2022

M-ZubairAhmed commented Feb 10, 2022

jgilliam17 commented Feb 10, 2022

mm-cloud-bot commented Feb 10, 2022

amyblais commented Feb 11, 2022

mattermod commented Feb 11, 2022

amyblais commented Mar 7, 2022

mattermod commented Mar 7, 2022

MM-41271 : Don't render sidebar channels when collapsed #9732

MM-41271 : Don't render sidebar channels when collapsed #9732

Conversation

M-ZubairAhmed commented Feb 1, 2022 • edited Loading

Summary

Ticket Link

Related Pull Requests

Screenshots

Release Note

hmhealey Feb 1, 2022

Choose a reason for hiding this comment

M-ZubairAhmed Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

hmhealey Feb 4, 2022

Choose a reason for hiding this comment

esethna commented Feb 2, 2022

hmhealey commented Feb 2, 2022

hmhealey Feb 4, 2022

Choose a reason for hiding this comment

M-ZubairAhmed Feb 6, 2022

Choose a reason for hiding this comment

M-ZubairAhmed Feb 6, 2022 • edited Loading

Choose a reason for hiding this comment

jespino left a comment

Choose a reason for hiding this comment

jgilliam17 commented Feb 9, 2022

M-ZubairAhmed commented Feb 10, 2022

jgilliam17 commented Feb 10, 2022

mm-cloud-bot commented Feb 10, 2022

amyblais commented Feb 11, 2022

mattermod commented Feb 11, 2022

amyblais commented Mar 7, 2022

mattermod commented Mar 7, 2022

M-ZubairAhmed commented Feb 1, 2022 •

edited

Loading

M-ZubairAhmed Feb 3, 2022 •

edited

Loading

M-ZubairAhmed Feb 6, 2022 •

edited

Loading