Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(dashboard): reduce rerenders of DragDroppable #16525

Merged
merged 2 commits into from
Sep 7, 2021
Merged

perf(dashboard): reduce rerenders of DragDroppable #16525

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a47361d
perf(dashboard): reduce rerenders of DragDroppable
kgabryje Aug 31, 2021
57e81e3
lint fix
kgabryje Sep 1, 2021
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
111 changes: 66 additions & 45 deletions superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/
/* eslint-env browser */
import cx from 'classnames';
import React, { FC } from 'react';
import React, { FC, useCallback, useMemo } from 'react';
import { JsonObject, styled, css } from '@superset-ui/core';
import ErrorBoundary from 'src/components/ErrorBoundary';
import BuilderComponentPane from 'src/dashboard/components/BuilderComponentPane';
Expand Down Expand Up @@ -157,23 +157,27 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
state => state.dashboardState.fullSizeChartId,
);

const handleChangeTab = ({
pathToTabIndex,
}: {
pathToTabIndex: string[];
}) => {
dispatch(setDirectPathToChild(pathToTabIndex));
};
const handleChangeTab = useCallback(
({ pathToTabIndex }: { pathToTabIndex: string[] }) => {
dispatch(setDirectPathToChild(pathToTabIndex));
},
[dispatch],
);

const handleDeleteTopLevelTabs = () => {
const handleDeleteTopLevelTabs = useCallback(() => {
dispatch(deleteTopLevelTabs());

const firstTab = getDirectPathToTabIndex(
getRootLevelTabsComponent(dashboardLayout),
0,
);
dispatch(setDirectPathToChild(firstTab));
};
}, [dashboardLayout, dispatch]);

const handleDrop = useCallback(
dropResult => dispatch(handleComponentDrop(dropResult)),
[dispatch],
);

const dashboardRoot = dashboardLayout[DASHBOARD_ROOT_ID];
const rootChildId = dashboardRoot.children[0];
Expand Down Expand Up @@ -217,6 +221,55 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
const filterBarHeight = `calc(100vh - ${offset}px)`;
const filterBarOffset = dashboardFiltersOpen ? 0 : barTopOffset + 20;

const draggableStyle = useMemo(
() => ({
marginLeft: dashboardFiltersOpen || editMode ? 0 : -32,
}),
[dashboardFiltersOpen, editMode],
);

const renderDraggableContent = useCallback(
({ dropIndicatorProps }: { dropIndicatorProps: JsonObject }) => (
<div>
{!hideDashboardHeader && <DashboardHeader />}
{dropIndicatorProps && <div {...dropIndicatorProps} />}
{!isReport && topLevelTabs && (
<WithPopoverMenu
shouldFocus={shouldFocusTabs}
menuItems={[
<IconButton
icon={<Icons.FallOutlined iconSize="xl" />}
label="Collapse tab content"
onClick={handleDeleteTopLevelTabs}
/>,
]}
editMode={editMode}
>
{/*
// @ts-ignore */}
Copy link
Member

Choose a reason for hiding this comment

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

This was here before, but wondering why this is here?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are some typing errors returned by DashboardComponent. We should definitely fix them... in a separate PR 🙂
I fixed those weird white spaces around ts-ignore though

<DashboardComponent
id={topLevelTabs?.id}
parentId={DASHBOARD_ROOT_ID}
depth={DASHBOARD_ROOT_DEPTH + 1}
index={0}
renderTabContent={false}
renderHoverMenu={false}
onChangeTab={handleChangeTab}
/>
</WithPopoverMenu>
)}
</div>
),
[
editMode,
handleChangeTab,
handleDeleteTopLevelTabs,
hideDashboardHeader,
isReport,
topLevelTabs,
],
);

return (
<StyledDiv>
{nativeFiltersEnabled && !editMode && (
Expand Down Expand Up @@ -244,45 +297,13 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
depth={DASHBOARD_ROOT_DEPTH}
index={0}
orientation="column"
onDrop={dropResult => dispatch(handleComponentDrop(dropResult))}
onDrop={handleDrop}
editMode={editMode}
// you cannot drop on/displace tabs if they already exist
disableDragDrop={!!topLevelTabs}
style={{
marginLeft: dashboardFiltersOpen || editMode ? 0 : -32,
}}
style={draggableStyle}
>
{({ dropIndicatorProps }: { dropIndicatorProps: JsonObject }) => (
<div>
{!hideDashboardHeader && <DashboardHeader />}
{dropIndicatorProps && <div {...dropIndicatorProps} />}
{!isReport && topLevelTabs && (
<WithPopoverMenu
shouldFocus={shouldFocusTabs}
menuItems={[
<IconButton
icon={<Icons.FallOutlined iconSize="xl" />}
label="Collapse tab content"
onClick={handleDeleteTopLevelTabs}
/>,
]}
editMode={editMode}
>
{/*
// @ts-ignore */}
<DashboardComponent
id={topLevelTabs?.id}
parentId={DASHBOARD_ROOT_ID}
depth={DASHBOARD_ROOT_DEPTH + 1}
index={0}
renderTabContent={false}
renderHoverMenu={false}
onChangeTab={handleChangeTab}
/>
</WithPopoverMenu>
)}
</div>
)}
{renderDraggableContent}
</DragDroppable>
</StyledHeader>
<StyledContent fullSizeChartId={fullSizeChartId}>
Expand Down
22 changes: 12 additions & 10 deletions superset-frontend/src/dashboard/components/DashboardGrid.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ const propTypes = {

const defaultProps = {};

const renderDraggableContentBottom = dropProps =>
dropProps.dropIndicatorProps && (
<div className="drop-indicator drop-indicator--bottom" />
);

const renderDraggableContentTop = dropProps =>
dropProps.dropIndicatorProps && (
<div className="drop-indicator drop-indicator--top" />
);

class DashboardGrid extends React.PureComponent {
constructor(props) {
super(props);
Expand Down Expand Up @@ -144,11 +154,7 @@ class DashboardGrid extends React.PureComponent {
className="empty-droptarget"
editMode
>
{({ dropIndicatorProps }) =>
dropIndicatorProps && (
<div className="drop-indicator drop-indicator--bottom" />
)
}
{renderDraggableContentBottom}
</DragDroppable>
)}

Expand Down Expand Up @@ -181,11 +187,7 @@ class DashboardGrid extends React.PureComponent {
className="empty-droptarget"
editMode
>
{({ dropIndicatorProps }) =>
dropIndicatorProps && (
<div className="drop-indicator drop-indicator--top" />
)
}
{renderDraggableContentTop}
</DragDroppable>
)}

Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/dashboard/components/dnd/DragDroppable.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ const defaultProps = {
};

// export unwrapped component for testing
export class UnwrappedDragDroppable extends React.Component {
export class UnwrappedDragDroppable extends React.PureComponent {
constructor(props) {
super(props);
this.state = {
Expand Down
22 changes: 12 additions & 10 deletions superset-frontend/src/dashboard/components/gridComponents/Tab.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,16 @@ const TabTitleContainer = styled.div`
`}
`;

const renderDraggableContentBottom = dropProps =>
dropProps.dropIndicatorProps && (
<div className="drop-indicator drop-indicator--bottom" />
);

const renderDraggableContentTop = dropProps =>
dropProps.dropIndicatorProps && (
<div className="drop-indicator drop-indicator--top" />
);

export default class Tab extends React.PureComponent {
constructor(props) {
super(props);
Expand Down Expand Up @@ -148,11 +158,7 @@ export default class Tab extends React.PureComponent {
editMode
className="empty-droptarget"
>
{({ dropIndicatorProps }) =>
dropIndicatorProps && (
<div className="drop-indicator drop-indicator--top" />
)
}
{renderDraggableContentTop}
</DragDroppable>
)}
{tabComponent.children.map((componentId, componentIndex) => (
Expand Down Expand Up @@ -184,11 +190,7 @@ export default class Tab extends React.PureComponent {
editMode
className="empty-droptarget"
>
{({ dropIndicatorProps }) =>
dropIndicatorProps && (
<div className="drop-indicator drop-indicator--bottom" />
)
}
{renderDraggableContentBottom}
</DragDroppable>
)}
</div>
Expand Down
14 changes: 8 additions & 6 deletions superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ export class Tabs extends React.PureComponent {
this.handleDeleteComponent = this.handleDeleteComponent.bind(this);
this.handleDeleteTab = this.handleDeleteTab.bind(this);
this.handleDropOnTab = this.handleDropOnTab.bind(this);
this.handleDrop = this.handleDrop.bind(this);
}

componentDidMount() {
Expand Down Expand Up @@ -281,6 +282,12 @@ export class Tabs extends React.PureComponent {
}
}

handleDrop(dropResult) {
if (dropResult.dragging.type !== TABS_TYPE) {
this.props.handleComponentDrop(dropResult);
}
}

render() {
const {
depth,
Expand All @@ -292,7 +299,6 @@ export class Tabs extends React.PureComponent {
onResizeStart,
onResize,
onResizeStop,
handleComponentDrop,
renderTabContent,
renderHoverMenu,
isComponentVisible: isCurrentTabVisible,
Expand All @@ -315,11 +321,7 @@ export class Tabs extends React.PureComponent {
orientation="row"
index={index}
depth={depth}
onDrop={dropResult => {
if (dropResult.dragging.type !== TABS_TYPE) {
handleComponentDrop(dropResult);
}
}}
onDrop={this.handleDrop}
editMode={editMode}
>
{({
Expand Down