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

MM-31275 New sidebar performance improvements #7207

Merged
merged 8 commits into from
Dec 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion actions/views/channel_sidebar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,14 @@ export function adjustTargetIndexForMove(state: GlobalState, categoryId: string,
}

export function clearChannelSelection() {
return (dispatch: DispatchFunc) => {
return (dispatch: DispatchFunc, getState: () => GlobalState) => {
const state = getState();

if (state.views.channelSidebar.multiSelectedChannelIds.length === 0) {
// No selection to clear
return Promise.resolve({data: true});
}

return dispatch({
type: ActionTypes.MULTISELECT_CHANNEL_CLEAR,
});
Expand Down
5 changes: 3 additions & 2 deletions components/legacy_sidebar/sidebar.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import ReactDOM from 'react-dom';
import {FormattedMessage, injectIntl} from 'react-intl';
import {PropTypes} from 'prop-types';
import classNames from 'classnames';
import throttle from 'lodash/throttle';

import Scrollbars from 'react-custom-scrollbars';
import {SpringSystem, MathUtil} from 'rebound';
Expand Down Expand Up @@ -270,9 +271,9 @@ class LegacySidebar extends React.PureComponent {
}
}

onScroll = () => {
onScroll = throttle(() => {
this.updateUnreadIndicators();
}
}, 100);

handleScrollAnimationUpdate = (spring) => {
const {scrollbar} = this.refs;
Expand Down
4 changes: 2 additions & 2 deletions components/sidebar/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {getLicense} from 'mattermost-redux/selectors/entities/general';
import {getBool} from 'mattermost-redux/selectors/entities/preferences';
import {haveIChannelPermission} from 'mattermost-redux/selectors/entities/roles';
import {getCurrentTeam} from 'mattermost-redux/selectors/entities/teams';
import {GenericAction, ActionFunc} from 'mattermost-redux/types/actions';
import {GenericAction} from 'mattermost-redux/types/actions';

import {createCategory, clearChannelSelection} from 'actions/views/channel_sidebar';
import {isUnreadFilterEnabled} from 'selectors/views/channel_sidebar';
Expand Down Expand Up @@ -66,7 +66,7 @@ type Actions = {

function mapDispatchToProps(dispatch: Dispatch<GenericAction>) {
return {
actions: bindActionCreators<ActionCreatorsMapObject<ActionFunc>, Actions>({
actions: bindActionCreators<ActionCreatorsMapObject, Actions>({
clearChannelSelection,
createCategory,
fetchMyCategories,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import Scrollbars from 'react-custom-scrollbars';
import {DragDropContext, Droppable, DropResult, DragStart, BeforeCapture} from 'react-beautiful-dnd';
import {Spring, SpringSystem} from 'rebound';
import classNames from 'classnames';
import debounce from 'lodash/debounce';
import throttle from 'lodash/throttle';

import {General} from 'mattermost-redux/constants';
import {Channel} from 'mattermost-redux/types/channels';
Expand Down Expand Up @@ -357,11 +357,11 @@ export default class SidebarChannelList extends React.PureComponent<Props, State
);
}

onScroll = () => {
onScroll = throttle(() => {
this.updateUnreadIndicators();
}
}, 100);

onTransitionEnd = debounce(() => {
onTransitionEnd = throttle(() => {
this.updateUnreadIndicators();
}, 100);

Expand Down
16 changes: 0 additions & 16 deletions components/sidebar/sidebar_menu/sidebar_menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,22 +59,6 @@ export default class SidebarMenu extends React.PureComponent<Props, State> {
}
}

// TODO: Temporary code to keep the menu in place while scrolling
// This shouldn't be necessary once the menus are fixed up
componentDidMount() {
Copy link
Member

Choose a reason for hiding this comment

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

Are these just not needed anymore?

Copy link
Member Author

@hmhealey hmhealey Dec 14, 2020

Choose a reason for hiding this comment

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

This is breaking the functionality that closes the menus when you scroll the sidebar, but I couldn't come up with a quick, low-risk way to do that with how the menus are written. I think the performance improvements of this removal heavily outweigh the slight UX degredation.

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 file a ticket so this can be revisited?

const scrollbars = document.querySelectorAll('#SidebarContainer .SidebarNavContainer .scrollbar--view');
if (scrollbars && scrollbars[0]) {
scrollbars[0].addEventListener('scroll', this.closeMenu);
}
}

componentWillUnmount() {
const scrollbars = document.querySelectorAll('#SidebarContainer .SidebarNavContainer .scrollbar--view');
if (scrollbars && scrollbars[0]) {
scrollbars[0].removeEventListener('scroll', this.closeMenu);
}
}

closeMenu = () => {
if (this.menuWrapperRef.current) {
this.menuWrapperRef.current.close();
Expand Down
13 changes: 13 additions & 0 deletions reducers/views/channel_sidebar.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,17 @@ describe('multiSelectedChannelIds', () => {
'channel-2',
]);
});

test('should not update state when clearing without a selection', () => {
const initialState: string[] = [];

const state = Reducers.multiSelectedChannelIds(
initialState,
{
type: ActionTypes.MULTISELECT_CHANNEL_CLEAR,
},
);

expect(state).toBe(initialState);
});
});
3 changes: 3 additions & 0 deletions reducers/views/channel_sidebar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,10 @@ export function multiSelectedChannelIds(state: string[] = [], action: GenericAct
return removeItem(state, action.data);
case ActionTypes.MULTISELECT_CHANNEL_TO:
return action.data;

case ActionTypes.MULTISELECT_CHANNEL_CLEAR:
return state.length > 0 ? [] : state;

case UserTypes.LOGOUT_SUCCESS:
return [];
default:
Expand Down