Skip to content

Commit

Permalink
Fix unnecessary rerendering of side content in desktop workspace (#2331)
Browse files Browse the repository at this point in the history 
* Memoize SideContent & MobileSideContent

* Update test snapshots
  • Loading branch information
@ianyong
ianyong authored Jan 29, 2023
1 parent 76c9f91 commit 1b16551
Show file tree
Hide file tree
Showing 4 changed files with 116 additions and 105 deletions.
20 changes: 10 additions & 10 deletions src/commons/assessmentWorkspace/__tests__/__snapshots__/AssessmentWorkspace.tsx.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ exports[`AssessmentWorkspace page with ContestVoting question renders correctly
<div className=\\"right-parent\\">
<Resizable bounds=\\"parent\\" className=\\"resize-side-content\\" enable={{...}} onResize={[Function: toggleDividerDisplay]} onResizeStop={[Function: onResizeStop]} as=\\"div\\" onResizeStart={[Function: onResizeStart]} style={{...}} grid={{...}} lockAspectRatio={false} lockAspectRatioExtraWidth={0} lockAspectRatioExtraHeight={0} scale={1} resizeRatio={1} snapGap={0}>
<div style={{...}} className=\\"resize-side-content\\">
<SideContent selectedTabId=\\"question_overview\\" tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\">
<Component selectedTabId=\\"question_overview\\" tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\">
<GenericSideContent tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\" renderFunction={[Function: renderFunction]}>
<div className=\\"side-content\\">
<Blueprint4.Card elevation={0} interactive={false}>
Expand Down Expand Up @@ -735,7 +735,7 @@ exports[`AssessmentWorkspace page with ContestVoting question renders correctly
</Blueprint4.Card>
</div>
</GenericSideContent>
</SideContent>
</Component>
<div className=\\"side-content-divider\\" />
<div className={[undefined]} style={[undefined]}>
<Resizer direction=\\"bottom\\" onResizeStart={[Function: bound ]} replaceStyles={[undefined]} className={[undefined]}>
Expand Down Expand Up @@ -1056,7 +1056,7 @@ exports[`AssessmentWorkspace page with MCQ question renders correctly 1`] = `
<div className=\\"right-parent\\">
<Resizable bounds=\\"parent\\" className=\\"resize-side-content\\" enable={{...}} onResize={[Function: toggleDividerDisplay]} onResizeStop={[Function: onResizeStop]} as=\\"div\\" onResizeStart={[Function: onResizeStart]} style={{...}} grid={{...}} lockAspectRatio={false} lockAspectRatioExtraWidth={0} lockAspectRatioExtraHeight={0} scale={1} resizeRatio={1} snapGap={0}>
<div style={{...}} className=\\"resize-side-content\\">
<SideContent selectedTabId=\\"question_overview\\" tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\">
<Component selectedTabId=\\"question_overview\\" tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\">
<GenericSideContent tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\" renderFunction={[Function: renderFunction]}>
<div className=\\"side-content\\">
<Blueprint4.Card elevation={0} interactive={false}>
Expand Down Expand Up @@ -1266,7 +1266,7 @@ exports[`AssessmentWorkspace page with MCQ question renders correctly 1`] = `
</Blueprint4.Card>
</div>
</GenericSideContent>
</SideContent>
</Component>
<div className=\\"side-content-divider\\" />
<div className={[undefined]} style={[undefined]}>
<Resizer direction=\\"bottom\\" onResizeStart={[Function: bound ]} replaceStyles={[undefined]} className={[undefined]}>
Expand Down Expand Up @@ -1589,7 +1589,7 @@ exports[`AssessmentWorkspace page with overdue assessment renders correctly 1`]
<div className=\\"right-parent\\">
<Resizable bounds=\\"parent\\" className=\\"resize-side-content\\" enable={{...}} onResize={[Function: toggleDividerDisplay]} onResizeStop={[Function: onResizeStop]} as=\\"div\\" onResizeStart={[Function: onResizeStart]} style={{...}} grid={{...}} lockAspectRatio={false} lockAspectRatioExtraWidth={0} lockAspectRatioExtraHeight={0} scale={1} resizeRatio={1} snapGap={0}>
<div style={{...}} className=\\"resize-side-content\\">
<SideContent selectedTabId=\\"question_overview\\" tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\">
<Component selectedTabId=\\"question_overview\\" tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\">
<GenericSideContent tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\" renderFunction={[Function: renderFunction]}>
<div className=\\"side-content\\">
<Blueprint4.Card elevation={0} interactive={false}>
Expand Down Expand Up @@ -1799,7 +1799,7 @@ exports[`AssessmentWorkspace page with overdue assessment renders correctly 1`]
</Blueprint4.Card>
</div>
</GenericSideContent>
</SideContent>
</Component>
<div className=\\"side-content-divider\\" />
<div className={[undefined]} style={[undefined]}>
<Resizer direction=\\"bottom\\" onResizeStart={[Function: bound ]} replaceStyles={[undefined]} className={[undefined]}>
Expand Down Expand Up @@ -2143,7 +2143,7 @@ exports[`AssessmentWorkspace page with programming question renders correctly 1`
<div className=\\"right-parent\\">
<Resizable bounds=\\"parent\\" className=\\"resize-side-content\\" enable={{...}} onResize={[Function: toggleDividerDisplay]} onResizeStop={[Function: onResizeStop]} as=\\"div\\" onResizeStart={[Function: onResizeStart]} style={{...}} grid={{...}} lockAspectRatio={false} lockAspectRatioExtraWidth={0} lockAspectRatioExtraHeight={0} scale={1} resizeRatio={1} snapGap={0}>
<div style={{...}} className=\\"resize-side-content\\">
<SideContent selectedTabId=\\"question_overview\\" tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\">
<Component selectedTabId=\\"question_overview\\" tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\">
<GenericSideContent tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\" renderFunction={[Function: renderFunction]}>
<div className=\\"side-content\\">
<Blueprint4.Card elevation={0} interactive={false}>
Expand Down Expand Up @@ -2353,7 +2353,7 @@ exports[`AssessmentWorkspace page with programming question renders correctly 1`
</Blueprint4.Card>
</div>
</GenericSideContent>
</SideContent>
</Component>
<div className=\\"side-content-divider\\" />
<div className={[undefined]} style={[undefined]}>
<Resizer direction=\\"bottom\\" onResizeStart={[Function: bound ]} replaceStyles={[undefined]} className={[undefined]}>
Expand Down Expand Up @@ -2697,7 +2697,7 @@ exports[`AssessmentWorkspace renders Grading tab correctly if the question has b
<div className=\\"right-parent\\">
<Resizable bounds=\\"parent\\" className=\\"resize-side-content\\" enable={{...}} onResize={[Function: toggleDividerDisplay]} onResizeStop={[Function: onResizeStop]} as=\\"div\\" onResizeStart={[Function: onResizeStart]} style={{...}} grid={{...}} lockAspectRatio={false} lockAspectRatioExtraWidth={0} lockAspectRatioExtraHeight={0} scale={1} resizeRatio={1} snapGap={0}>
<div style={{...}} className=\\"resize-side-content\\">
<SideContent selectedTabId=\\"grading\\" tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\">
<Component selectedTabId=\\"grading\\" tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\">
<GenericSideContent tabs={{...}} onChange={[Function: onChangeTabs]} workspaceLocation=\\"assessment\\" renderFunction={[Function: renderFunction]}>
<div className=\\"side-content\\">
<Blueprint4.Card elevation={0} interactive={false}>
Expand Down Expand Up @@ -3005,7 +3005,7 @@ exports[`AssessmentWorkspace renders Grading tab correctly if the question has b
</Blueprint4.Card>
</div>
</GenericSideContent>
</SideContent>
</Component>
<div className=\\"side-content-divider\\" />
<div className={[undefined]} style={[undefined]}>
<Resizer direction=\\"bottom\\" onResizeStart={[Function: bound ]} replaceStyles={[undefined]} className={[undefined]}>
Expand Down
129 changes: 65 additions & 64 deletions src/commons/mobileWorkspace/mobileSideContent/MobileSideContent.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import GenericSideContent, {
GenericSideContentProps
} from '../../sideContent/GenericSideContent';
import { SideContentTab, SideContentType } from '../../sideContent/SideContentTypes';
import { propsAreEqual } from '../../utils/MemoizeHelper';
import { WorkspaceLocation } from '../../workspace/WorkspaceTypes';
import MobileControlBar from './MobileControlBar';

Expand Down Expand Up @@ -56,76 +57,76 @@ const renderTab = (tab: SideContentTab, isIOS: boolean, workspaceLocation?: Work
);
};

const MobileSideContent: React.FC<MobileSideContentProps> = ({
selectedTabId,
renderActiveTabPanelOnly,
mobileControlBarProps,
...otherProps
}) => {
const isIOS = /iPhone|iPod/.test(navigator.platform);
const MobileSideContent: React.FC<MobileSideContentProps> = React.memo(
({ selectedTabId, renderActiveTabPanelOnly, mobileControlBarProps, ...otherProps }) => {
const isIOS = /iPhone|iPod/.test(navigator.platform);

/**
* renderedPanels is not memoized since a change in selectedTabId (when changing tabs)
* would force React.useMemo to recompute the nullary function anyway
*/
const renderedPanels = (dynamicTabs: SideContentTab[]) => {
// TODO: Fix the CSS of all the panels (e.g. subst_visualizer)
const renderPanel = (tab: SideContentTab, workspaceLocation?: WorkspaceLocation) => {
if (!tab.body) return;
/**
* renderedPanels is not memoized since a change in selectedTabId (when changing tabs)
* would force React.useMemo to recompute the nullary function anyway
*/
const renderedPanels = (dynamicTabs: SideContentTab[]) => {
// TODO: Fix the CSS of all the panels (e.g. subst_visualizer)
const renderPanel = (tab: SideContentTab, workspaceLocation?: WorkspaceLocation) => {
if (!tab.body) return;

const tabBody: JSX.Element = workspaceLocation
? {
...tab.body,
props: {
...tab.body.props,
workspaceLocation
const tabBody: JSX.Element = workspaceLocation
? {
...tab.body,
props: {
...tab.body.props,
workspaceLocation
}
}
}
: tab.body;
: tab.body;

// Render the other panels only when their corresponding tab is selected
return (
<div
className={tab.id === selectedTabId ? 'mobile-selected-panel' : 'mobile-unselected-panel'}
key={tab.id}
>
{tabBody}
</div>
);
};
// Render the other panels only when their corresponding tab is selected
return (
<div
className={
tab.id === selectedTabId ? 'mobile-selected-panel' : 'mobile-unselected-panel'
}
key={tab.id}
>
{tabBody}
</div>
);
};

return dynamicTabs.map(tab => renderPanel(tab, otherProps.workspaceLocation));
};
return dynamicTabs.map(tab => renderPanel(tab, otherProps.workspaceLocation));
};

return (
<GenericSideContent
{...otherProps}
renderFunction={(dynamicTabs, changeTabsCallback) => {
return (
<>
{renderedPanels(dynamicTabs)}
<div className="mobile-tabs-container">
<Tabs
id="mobile-side-content"
onChange={changeTabsCallback}
renderActiveTabPanelOnly={renderActiveTabPanelOnly}
selectedTabId={selectedTabId}
className={classNames(Classes.DARK, 'mobile-side-content')}
>
{dynamicTabs.map(tab => renderTab(tab, isIOS, otherProps.workspaceLocation))}
return (
<GenericSideContent
{...otherProps}
renderFunction={(dynamicTabs, changeTabsCallback) => {
return (
<>
{renderedPanels(dynamicTabs)}
<div className="mobile-tabs-container">
<Tabs
id="mobile-side-content"
onChange={changeTabsCallback}
renderActiveTabPanelOnly={renderActiveTabPanelOnly}
selectedTabId={selectedTabId}
className={classNames(Classes.DARK, 'mobile-side-content')}
>
{dynamicTabs.map(tab => renderTab(tab, isIOS, otherProps.workspaceLocation))}

{/* Render the bottom ControlBar 'Cog' button only in the Playground or Sicp Workspace */}
{(otherProps.workspaceLocation === 'playground' ||
otherProps.workspaceLocation === 'sicp') && (
<MobileControlBar {...mobileControlBarProps} />
)}
</Tabs>
</div>
</>
);
}}
/>
);
};
{/* Render the bottom ControlBar 'Cog' button only in the Playground or Sicp Workspace */}
{(otherProps.workspaceLocation === 'playground' ||
otherProps.workspaceLocation === 'sicp') && (
<MobileControlBar {...mobileControlBarProps} />
)}
</Tabs>
</div>
</>
);
}}
/>
);
},
propsAreEqual
);

export default MobileSideContent;
60 changes: 29 additions & 31 deletions src/commons/sideContent/SideContent.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Card, Icon, Tab, TabProps, Tabs } from '@blueprintjs/core';
import { Tooltip2 } from '@blueprintjs/popover2';
import * as React from 'react';

import { propsAreEqual } from '../utils/MemoizeHelper';
import { assertType } from '../utils/TypeHelper';
import { WorkspaceLocation } from '../workspace/WorkspaceTypes';
import GenericSideContent, { generateIconId, GenericSideContentProps } from './GenericSideContent';
Expand Down Expand Up @@ -81,36 +82,33 @@ const renderTab = (
return <Tab key={tabId} {...tabProps} panel={tabPanel} />;
};

const SideContent: React.FC<SideContentProps> = ({
selectedTabId,
renderActiveTabPanelOnly,
editorWidth,
sideContentHeight,
...otherProps
}) => {
return (
<GenericSideContent
{...otherProps}
renderFunction={(dynamicTabs, changeTabsCallback) => (
<div className="side-content">
<Card>
<div className="side-content-tabs">
<Tabs
id="side-content-tabs"
onChange={changeTabsCallback}
renderActiveTabPanelOnly={renderActiveTabPanelOnly}
selectedTabId={selectedTabId}
>
{dynamicTabs.map(tab =>
renderTab(tab, otherProps.workspaceLocation, editorWidth, sideContentHeight)
)}
</Tabs>
</div>
</Card>
</div>
)}
/>
);
};
const SideContent: React.FC<SideContentProps> = React.memo(
({ selectedTabId, renderActiveTabPanelOnly, editorWidth, sideContentHeight, ...otherProps }) => {
return (
<GenericSideContent
{...otherProps}
renderFunction={(dynamicTabs, changeTabsCallback) => (
<div className="side-content">
<Card>
<div className="side-content-tabs">
<Tabs
id="side-content-tabs"
onChange={changeTabsCallback}
renderActiveTabPanelOnly={renderActiveTabPanelOnly}
selectedTabId={selectedTabId}
>
{dynamicTabs.map(tab =>
renderTab(tab, otherProps.workspaceLocation, editorWidth, sideContentHeight)
)}
</Tabs>
</div>
</Card>
</div>
)}
/>
);
},
propsAreEqual
);

export default SideContent;
12 changes: 12 additions & 0 deletions src/commons/utils/MemoizeHelper.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import * as _ from 'lodash';

/**
* Performs a deep comparison between the previous & next props state
* to determine if they are equivalent. For use with the `React.memo`
* higher order component.
*
* @param prevProps The previous state of the props passed into a component.
* @param nextProps The next state of the props passed into a component
*/
export const propsAreEqual = <T>(prevProps: T, nextProps: T): boolean =>
_.isEqual(prevProps, nextProps);

0 comments on commit 1b16551

Please sign in to comment.