This is an automated email from the ASF dual-hosted git repository. michaelsmolina pushed a commit to branch 4.0 in repository https://gitbox.apache.org/repos/asf/superset.git
commit 3440a301baf1dc205b87f3f4f8f4502a48846b44 Author: JUST.in DO IT <[email protected]> AuthorDate: Wed Feb 21 11:29:34 2024 -0800 fix(dashboard): drag and drop indicator UX (#26699) (cherry picked from commit ac8c283df04b6c4cbc24a5ae625e05a8f2679802) --- .../DashboardBuilder/DashboardBuilder.tsx | 53 ++++--- .../DashboardBuilder/DashboardWrapper.test.tsx | 18 ++- .../DashboardBuilder/DashboardWrapper.tsx | 33 +++- .../src/dashboard/components/DashboardGrid.jsx | 115 ++++++++------ .../src/dashboard/components/dnd/DragDroppable.jsx | 56 ++----- .../src/dashboard/components/dnd/handleDrop.js | 32 ++-- .../components/gridComponents/ChartHolder.tsx | 9 +- .../dashboard/components/gridComponents/Column.jsx | 118 +++++++++++--- .../components/gridComponents/Column.test.jsx | 23 ++- .../components/gridComponents/Divider.jsx | 11 +- .../components/gridComponents/Divider.test.jsx | 6 +- .../components/gridComponents/DynamicComponent.tsx | 9 +- .../dashboard/components/gridComponents/Header.jsx | 10 +- .../components/gridComponents/Header.test.jsx | 6 +- .../components/gridComponents/Markdown.jsx | 9 +- .../components/gridComponents/Markdown.test.jsx | 8 +- .../dashboard/components/gridComponents/Row.jsx | 175 +++++++++++++++++---- .../components/gridComponents/Row.test.jsx | 23 ++- .../dashboard/components/gridComponents/Tab.jsx | 93 ++++++----- .../components/gridComponents/Tab.test.tsx | 59 ++++--- .../dashboard/components/gridComponents/Tabs.jsx | 19 +-- .../components/gridComponents/Tabs.test.jsx | 14 +- .../components/gridComponents/Tabs.test.tsx | 12 +- superset-frontend/src/dashboard/constants.ts | 1 + .../src/dashboard/util/getDropPosition.js | 3 +- .../src/dashboard/util/getDropPosition.test.js | 5 +- 26 files changed, 585 insertions(+), 335 deletions(-) diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx index 2756e71359..e86d924b21 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx @@ -44,7 +44,7 @@ import BuilderComponentPane from 'src/dashboard/components/BuilderComponentPane' import DashboardHeader from 'src/dashboard/containers/DashboardHeader'; import Icons from 'src/components/Icons'; import IconButton from 'src/dashboard/components/IconButton'; -import DragDroppable from 'src/dashboard/components/dnd/DragDroppable'; +import { Droppable } from 'src/dashboard/components/dnd/DragDroppable'; import DashboardComponent from 'src/dashboard/containers/DashboardComponent'; import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu'; import getDirectPathToTabIndex from 'src/dashboard/util/getDirectPathToTabIndex'; @@ -81,6 +81,7 @@ import { MAIN_HEADER_HEIGHT, OPEN_FILTER_BAR_MAX_WIDTH, OPEN_FILTER_BAR_WIDTH, + EMPTY_CONTAINER_Z_INDEX, } from 'src/dashboard/constants'; import { getRootLevelTabsComponent, shouldFocusTabs } from './utils'; import DashboardContainer from './DashboardContainer'; @@ -107,12 +108,27 @@ const StickyPanel = styled.div<{ width: number }>` // @z-index-above-dashboard-popovers (99) + 1 = 100 const StyledHeader = styled.div` - grid-column: 2; - grid-row: 1; - position: sticky; - top: 0; - z-index: 100; - max-width: 100vw; + ${({ theme }) => css` + grid-column: 2; + grid-row: 1; + position: sticky; + top: 0; + z-index: 100; + max-width: 100vw; + + .empty-droptarget:before { + position: absolute; + content: ''; + display: none; + width: calc(100% - ${theme.gridUnit * 2}px); + height: calc(100% - ${theme.gridUnit * 2}px); + left: ${theme.gridUnit}px; + top: ${theme.gridUnit}px; + border: 1px dashed transparent; + border-radius: ${theme.gridUnit}px; + opacity: 0.5; + } + `} `; const StyledContent = styled.div<{ @@ -211,13 +227,9 @@ const DashboardContentWrapper = styled.div` /* provide hit area in case row contents is edge to edge */ .dashboard-component-tabs-content { - .dragdroppable-row { + > .dragdroppable-row { padding-top: ${theme.gridUnit * 4}px; } - - & > div:not(:last-child):not(.empty-droptarget) { - margin-bottom: ${theme.gridUnit * 4}px; - } } .dashboard-component-chart-holder { @@ -250,25 +262,21 @@ const DashboardContentWrapper = styled.div` } & > .empty-droptarget { + z-index: ${EMPTY_CONTAINER_Z_INDEX}; position: absolute; width: 100%; } & > .empty-droptarget:first-child:not(.empty-droptarget--full) { height: ${theme.gridUnit * 4}px; - top: -2px; - z-index: 10; + top: 0; } & > .empty-droptarget:last-child { - height: ${theme.gridUnit * 3}px; - bottom: 0; + height: ${theme.gridUnit * 4}px; + bottom: ${-theme.gridUnit * 4}px; } } - - .empty-droptarget:first-child .drop-indicator--bottom { - top: ${theme.gridUnit * 6}px; - } `} `; @@ -616,8 +624,9 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => { )} <StyledHeader ref={headerRef}> {/* @ts-ignore */} - <DragDroppable + <Droppable data-test="top-level-tabs" + className={cx(!topLevelTabs && editMode && 'empty-droptarget')} component={dashboardRoot} parentComponent={null} depth={DASHBOARD_ROOT_DEPTH} @@ -630,7 +639,7 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => { style={draggableStyle} > {renderDraggableContent} - </DragDroppable> + </Droppable> </StyledHeader> <StyledContent fullSizeChartId={fullSizeChartId}> <Global diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.test.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.test.tsx index fb913b4627..add3b482fd 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.test.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.test.tsx @@ -17,11 +17,19 @@ * under the License. */ import React from 'react'; -import { fireEvent, render } from 'spec/helpers/testing-library'; +import { fireEvent, render, waitFor } from 'spec/helpers/testing-library'; import { OptionControlLabel } from 'src/explore/components/controls/OptionControls'; import DashboardWrapper from './DashboardWrapper'; +beforeAll(() => { + jest.useFakeTimers(); +}); + +afterAll(() => { + jest.useRealTimers(); +}); + test('should render children', () => { const { getByTestId } = render( <DashboardWrapper> @@ -32,7 +40,7 @@ test('should render children', () => { expect(getByTestId('mock-children')).toBeInTheDocument(); }); -test('should update the style on dragging state', () => { +test('should update the style on dragging state', async () => { const defaultProps = { label: <span>Test label</span>, tooltipTitle: 'This is a tooltip title', @@ -69,7 +77,13 @@ test('should update the style on dragging state', () => { container.getElementsByClassName('dragdroppable--dragging'), ).toHaveLength(0); fireEvent.dragStart(getByText('Label 1')); + await waitFor(() => jest.runAllTimers()); expect( container.getElementsByClassName('dragdroppable--dragging'), ).toHaveLength(1); + fireEvent.dragEnd(getByText('Label 1')); + // immediately discards dragging state after dragEnd + expect( + container.getElementsByClassName('dragdroppable--dragging'), + ).toHaveLength(0); }); diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx index 5bb193de1b..2bcdc5b9cd 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx @@ -17,11 +17,12 @@ * under the License. */ import React, { useEffect } from 'react'; -import { css, styled } from '@superset-ui/core'; +import { FAST_DEBOUNCE, css, styled } from '@superset-ui/core'; import { RootState } from 'src/dashboard/types'; import { useSelector } from 'react-redux'; import { useDragDropManager } from 'react-dnd'; import classNames from 'classnames'; +import { debounce } from 'lodash'; const StyledDiv = styled.div` ${({ theme }) => css` @@ -32,10 +33,20 @@ const StyledDiv = styled.div` flex: 1; /* Special cases */ - &.dragdroppable--dragging - .dashboard-component-tabs-content - > .empty-droptarget.empty-droptarget--full { - height: 100%; + &.dragdroppable--dragging { + & + .dashboard-component-tabs-content + > .empty-droptarget.empty-droptarget--full { + height: 100%; + } + & .empty-droptarget:before { + display: block; + border-color: ${theme.colors.primary.light1}; + background-color: ${theme.colors.primary.light3}; + } + & .grid-row:after { + border-style: hidden; + } } /* A row within a column has inset hover menu */ @@ -106,12 +117,22 @@ const DashboardWrapper: React.FC<Props> = ({ children }) => { useEffect(() => { const monitor = dragDropManager.getMonitor(); + const debouncedSetIsDragged = debounce(setIsDragged, FAST_DEBOUNCE); const unsub = monitor.subscribeToStateChange(() => { - setIsDragged(monitor.isDragging()); + const isDragging = monitor.isDragging(); + if (isDragging) { + // set a debounced function to prevent HTML5 drag source + // from interfering with the drop zone highlighting + debouncedSetIsDragged(true); + } else { + debouncedSetIsDragged.cancel(); + setIsDragged(false); + } }); return () => { unsub(); + debouncedSetIsDragged.cancel(); }; }, [dragDropManager]); diff --git a/superset-frontend/src/dashboard/components/DashboardGrid.jsx b/superset-frontend/src/dashboard/components/DashboardGrid.jsx index 70cf65218f..93444c7423 100644 --- a/superset-frontend/src/dashboard/components/DashboardGrid.jsx +++ b/superset-frontend/src/dashboard/components/DashboardGrid.jsx @@ -23,7 +23,7 @@ import { addAlpha, css, styled, t } from '@superset-ui/core'; import { EmptyStateBig } from 'src/components/EmptyState'; import { componentShape } from '../util/propShapes'; import DashboardComponent from '../containers/DashboardComponent'; -import DragDroppable from './dnd/DragDroppable'; +import { Droppable } from './dnd/DragDroppable'; import { GRID_GUTTER_SIZE, GRID_COLUMN_COUNT } from '../util/constants'; import { TAB_TYPE } from '../util/componentTypes'; @@ -41,15 +41,8 @@ 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" /> - ); +const renderDraggableContent = dropProps => + dropProps.dropIndicatorProps && <div {...dropProps.dropIndicatorProps} />; const DashboardEmptyStateContainer = styled.div` position: absolute; @@ -60,28 +53,42 @@ const DashboardEmptyStateContainer = styled.div` `; const GridContent = styled.div` - ${({ theme }) => css` + ${({ theme, editMode }) => css` display: flex; flex-direction: column; /* gutters between rows */ & > div:not(:last-child):not(.empty-droptarget) { - margin-bottom: ${theme.gridUnit * 4}px; + ${!editMode && `margin-bottom: ${theme.gridUnit * 4}px`}; } - & > .empty-droptarget { + .empty-droptarget { width: 100%; - height: 100%; + height: ${theme.gridUnit * 4}px; + display: flex; + align-items: center; + justify-content: center; + border-radius: ${theme.gridUnit}px; + overflow: hidden; + + &:before { + content: ''; + display: block; + width: calc(100% - ${theme.gridUnit * 2}px); + height: calc(100% - ${theme.gridUnit * 2}px); + border: 1px dashed transparent; + border-radius: ${theme.gridUnit}px; + opacity: 0.5; + } } & > .empty-droptarget:first-child { - height: ${theme.gridUnit * 12}px; - margin-top: ${theme.gridUnit * -6}px; + height: ${theme.gridUnit * 4}px; + margin-top: ${theme.gridUnit * -4}px; } & > .empty-droptarget:last-child { - height: ${theme.gridUnit * 12}px; - margin-top: ${theme.gridUnit * -6}px; + height: ${theme.gridUnit * 24}px; } & > .empty-droptarget.empty-droptarget--full:only-child { @@ -265,10 +272,14 @@ class DashboardGrid extends React.PureComponent { </DashboardEmptyStateContainer> )} <div className="dashboard-grid" ref={this.setGridRef}> - <GridContent className="grid-content" data-test="grid-content"> + <GridContent + className="grid-content" + data-test="grid-content" + editMode={editMode} + > {/* make the area above components droppable */} {editMode && ( - <DragDroppable + <Droppable component={gridComponent} depth={depth} parentComponent={null} @@ -281,41 +292,43 @@ class DashboardGrid extends React.PureComponent { gridComponent?.children?.length === 0, })} editMode + dropToChild={gridComponent?.children?.length === 0} > - {renderDraggableContentTop} - </DragDroppable> + {renderDraggableContent} + </Droppable> )} {gridComponent?.children?.map((id, index) => ( - <DashboardComponent - key={id} - id={id} - parentId={gridComponent.id} - depth={depth + 1} - index={index} - availableColumnCount={GRID_COLUMN_COUNT} - columnWidth={columnWidth} - isComponentVisible={isComponentVisible} - onResizeStart={this.handleResizeStart} - onResize={this.handleResize} - onResizeStop={this.handleResizeStop} - onChangeTab={this.handleChangeTab} - /> + <React.Fragment key={id}> + <DashboardComponent + id={id} + parentId={gridComponent.id} + depth={depth + 1} + index={index} + availableColumnCount={GRID_COLUMN_COUNT} + columnWidth={columnWidth} + isComponentVisible={isComponentVisible} + onResizeStart={this.handleResizeStart} + onResize={this.handleResize} + onResizeStop={this.handleResizeStop} + onChangeTab={this.handleChangeTab} + /> + {/* make the area below components droppable */} + {editMode && ( + <Droppable + component={gridComponent} + depth={depth} + parentComponent={null} + index={index + 1} + orientation="column" + onDrop={handleComponentDrop} + className="empty-droptarget" + editMode + > + {renderDraggableContent} + </Droppable> + )} + </React.Fragment> ))} - {/* make the area below components droppable */} - {editMode && gridComponent?.children?.length > 0 && ( - <DragDroppable - component={gridComponent} - depth={depth} - parentComponent={null} - index={gridComponent.children.length} - orientation="column" - onDrop={handleComponentDrop} - className="empty-droptarget" - editMode - > - {renderDraggableContentBottom} - </DragDroppable> - )} {isResizing && Array(GRID_COLUMN_COUNT) .fill(null) diff --git a/superset-frontend/src/dashboard/components/dnd/DragDroppable.jsx b/superset-frontend/src/dashboard/components/dnd/DragDroppable.jsx index 6a49f98875..185b4c08c6 100644 --- a/superset-frontend/src/dashboard/components/dnd/DragDroppable.jsx +++ b/superset-frontend/src/dashboard/components/dnd/DragDroppable.jsx @@ -25,12 +25,7 @@ import { css, styled } from '@superset-ui/core'; import { componentShape } from '../../util/propShapes'; import { dragConfig, dropConfig } from './dragDroppableConfig'; -import { - DROP_TOP, - DROP_RIGHT, - DROP_BOTTOM, - DROP_LEFT, -} from '../../util/getDropPosition'; +import { DROP_FORBIDDEN } from '../../util/getDropPosition'; const propTypes = { children: PropTypes.func, @@ -39,6 +34,7 @@ const propTypes = { parentComponent: componentShape, depth: PropTypes.number.isRequired, disableDragDrop: PropTypes.bool, + dropToChild: PropTypes.bool, orientation: PropTypes.oneOf(['row', 'column']), index: PropTypes.number.isRequired, style: PropTypes.object, @@ -61,6 +57,7 @@ const defaultProps = { style: null, parentComponent: null, disableDragDrop: false, + dropToChild: false, children() {}, onDrop() {}, onHover() {}, @@ -90,49 +87,18 @@ const DragDroppableStyles = styled.div` z-index: 10; } - &.empty-droptarget--full > .drop-indicator--top { - height: 100%; - opacity: 0.3; - } - & { .drop-indicator { display: block; background-color: ${theme.colors.primary.base}; position: absolute; z-index: 10; - } - - .drop-indicator--top { - top: ${-theme.gridUnit - 2}px; - left: 0; - height: ${theme.gridUnit}px; - width: 100%; - min-width: ${theme.gridUnit * 4}px; - } - - .drop-indicator--bottom { - bottom: ${-theme.gridUnit - 2}px; - left: 0; - height: ${theme.gridUnit}px; + opacity: 0.3; width: 100%; - min-width: ${theme.gridUnit * 4}px; - } - - .drop-indicator--right { - top: 0; - left: calc(100% - ${theme.gridUnit}px); height: 100%; - width: ${theme.gridUnit}px; - min-height: ${theme.gridUnit * 4}px; - } - - .drop-indicator--left { - top: 0; - left: 0; - height: 100%; - width: ${theme.gridUnit}px; - min-height: ${theme.gridUnit * 4}px; + &.drop-indicator--forbidden { + background-color: ${theme.colors.error.light1}; + } } } `}; @@ -189,10 +155,7 @@ export class UnwrappedDragDroppable extends React.PureComponent { ? { className: cx( 'drop-indicator', - dropIndicator === DROP_TOP && 'drop-indicator--top', - dropIndicator === DROP_BOTTOM && 'drop-indicator--bottom', - dropIndicator === DROP_LEFT && 'drop-indicator--left', - dropIndicator === DROP_RIGHT && 'drop-indicator--right', + dropIndicator === DROP_FORBIDDEN && 'drop-indicator--forbidden', ), } : null; @@ -226,6 +189,9 @@ export class UnwrappedDragDroppable extends React.PureComponent { UnwrappedDragDroppable.propTypes = propTypes; UnwrappedDragDroppable.defaultProps = defaultProps; +export const Draggable = DragSource(...dragConfig)(UnwrappedDragDroppable); +export const Droppable = DropTarget(...dropConfig)(UnwrappedDragDroppable); + // note that the composition order here determines using // component.method() vs decoratedComponentInstance.method() in the drag/drop config export default DragSource(...dragConfig)( diff --git a/superset-frontend/src/dashboard/components/dnd/handleDrop.js b/superset-frontend/src/dashboard/components/dnd/handleDrop.js index 450a60867c..6d50d39c45 100644 --- a/superset-frontend/src/dashboard/components/dnd/handleDrop.js +++ b/superset-frontend/src/dashboard/components/dnd/handleDrop.js @@ -18,10 +18,7 @@ */ import getDropPosition, { clearDropCache, - DROP_TOP, - DROP_RIGHT, - DROP_BOTTOM, - DROP_LEFT, + DROP_FORBIDDEN, } from '../../util/getDropPosition'; export default function handleDrop(props, monitor, Component) { @@ -31,7 +28,7 @@ export default function handleDrop(props, monitor, Component) { Component.setState(() => ({ dropIndicator: null })); const dropPosition = getDropPosition(monitor, Component); - if (!dropPosition) { + if (!dropPosition || dropPosition === DROP_FORBIDDEN) { return undefined; } @@ -40,19 +37,11 @@ export default function handleDrop(props, monitor, Component) { component, index: componentIndex, onDrop, - orientation, + dropToChild, } = Component.props; const draggingItem = monitor.getItem(); - const dropAsChildOrSibling = - (orientation === 'row' && - (dropPosition === DROP_TOP || dropPosition === DROP_BOTTOM)) || - (orientation === 'column' && - (dropPosition === DROP_LEFT || dropPosition === DROP_RIGHT)) - ? 'sibling' - : 'child'; - const dropResult = { source: { id: draggingItem.parentId, @@ -67,12 +56,18 @@ export default function handleDrop(props, monitor, Component) { }; // simplest case, append as child - if (dropAsChildOrSibling === 'child') { + if (dropToChild) { dropResult.destination = { id: component.id, type: component.type, index: component.children.length, }; + } else if (!parentComponent) { + dropResult.destination = { + id: component.id, + type: component.type, + index: componentIndex, + }; } else { // if the item is in the same list with a smaller index, you must account for the // "missing" index upon movement within the list @@ -81,10 +76,9 @@ export default function handleDrop(props, monitor, Component) { const sameParentLowerIndex = sameParent && draggingItem.index < componentIndex; - let nextIndex = sameParentLowerIndex ? componentIndex - 1 : componentIndex; - if (dropPosition === DROP_BOTTOM || dropPosition === DROP_RIGHT) { - nextIndex += 1; - } + const nextIndex = sameParentLowerIndex + ? componentIndex - 1 + : componentIndex; dropResult.destination = { id: parentComponent.id, diff --git a/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx b/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx index a93f3b0b8d..bcc58d0691 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx @@ -25,7 +25,7 @@ import { LayoutItem, RootState } from 'src/dashboard/types'; import AnchorLink from 'src/dashboard/components/AnchorLink'; import Chart from 'src/dashboard/containers/Chart'; import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton'; -import DragDroppable from 'src/dashboard/components/dnd/DragDroppable'; +import { Draggable } from 'src/dashboard/components/dnd/DragDroppable'; import HoverMenu from 'src/dashboard/components/menu/HoverMenu'; import ResizableContainer from 'src/dashboard/components/resizable/ResizableContainer'; import getChartAndLabelComponentIdFromPath from 'src/dashboard/util/getChartAndLabelComponentIdFromPath'; @@ -243,7 +243,7 @@ const ChartHolder: React.FC<ChartHolderProps> = ({ }, []); return ( - <DragDroppable + <Draggable component={component} parentComponent={parentComponent} orientation={parentComponent.type === ROW_TYPE ? 'column' : 'row'} @@ -253,7 +253,7 @@ const ChartHolder: React.FC<ChartHolderProps> = ({ disableDragDrop={false} editMode={editMode} > - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => ( <ResizableContainer id={component.id} adjustableWidth={parentComponent.type === ROW_TYPE} @@ -324,10 +324,9 @@ const ChartHolder: React.FC<ChartHolderProps> = ({ </HoverMenu> )} </div> - {dropIndicatorProps && <div {...dropIndicatorProps} />} </ResizableContainer> )} - </DragDroppable> + </Draggable> ); }; diff --git a/superset-frontend/src/dashboard/components/gridComponents/Column.jsx b/superset-frontend/src/dashboard/components/gridComponents/Column.jsx index 1883531404..3511c54a99 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Column.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Column.jsx @@ -23,7 +23,10 @@ import { css, styled, t } from '@superset-ui/core'; import Icons from 'src/components/Icons'; import DashboardComponent from 'src/dashboard/containers/DashboardComponent'; import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton'; -import DragDroppable from 'src/dashboard/components/dnd/DragDroppable'; +import { + Draggable, + Droppable, +} from 'src/dashboard/components/dnd/DragDroppable'; import DragHandle from 'src/dashboard/components/dnd/DragHandle'; import HoverMenu from 'src/dashboard/components/menu/HoverMenu'; import IconButton from 'src/dashboard/components/IconButton'; @@ -33,6 +36,7 @@ import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu'; import backgroundStyleOptions from 'src/dashboard/util/backgroundStyleOptions'; import { componentShape } from 'src/dashboard/util/propShapes'; import { BACKGROUND_TRANSPARENT } from 'src/dashboard/util/constants'; +import { EMPTY_CONTAINER_Z_INDEX } from 'src/dashboard/constants'; const propTypes = { id: PropTypes.string.isRequired, @@ -60,13 +64,13 @@ const propTypes = { const defaultProps = {}; const ColumnStyles = styled.div` - ${({ theme }) => css` + ${({ theme, editMode }) => css` &.grid-column { width: 100%; position: relative; & > :not(.hover-menu):not(:last-child) { - margin-bottom: ${theme.gridUnit * 4}px; + ${!editMode && `margin-bottom: ${theme.gridUnit * 4}px;`} } } @@ -86,6 +90,25 @@ const ColumnStyles = styled.div` border: 1px dashed ${theme.colors.primary.base}; z-index: 2; } + + & .empty-droptarget { + &.droptarget-edge { + position: absolute; + z-index: ${EMPTY_CONTAINER_Z_INDEX}; + &:first-child { + inset-block-start: 0; + } + &:last-child { + inset-block-end: 0; + } + } + &:first-child:not(.droptarget-edge) { + position: absolute; + z-index: ${EMPTY_CONTAINER_Z_INDEX}; + width: 100%; + height: 100%; + } + } `} `; @@ -163,7 +186,7 @@ class Column extends React.PureComponent { ); return ( - <DragDroppable + <Draggable component={columnComponent} parentComponent={parentComponent} orientation="column" @@ -172,7 +195,7 @@ class Column extends React.PureComponent { onDrop={handleComponentDrop} editMode={editMode} > - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => ( <ResizableContainer id={columnComponent.id} adjustableWidth @@ -215,34 +238,85 @@ class Column extends React.PureComponent { )} <ColumnStyles className={cx('grid-column', backgroundStyle.className)} + editMode={editMode} > + {editMode && ( + <Droppable + component={columnComponent} + parentComponent={parentComponent} + {...(columnItems.length === 0 + ? { + component: columnComponent, + parentComponent, + dropToChild: true, + } + : { + component: columnItems, + parentComponent: columnComponent, + })} + depth={depth + 1} + index={0} + orientation="column" + onDrop={handleComponentDrop} + className={cx( + 'empty-droptarget', + columnItems.length > 0 && 'droptarget-edge', + )} + editMode + > + {({ dropIndicatorProps }) => + dropIndicatorProps && <div {...dropIndicatorProps} /> + } + </Droppable> + )} {columnItems.length === 0 ? ( <div css={emptyColumnContentStyles}>{t('Empty column')}</div> ) : ( columnItems.map((componentId, itemIndex) => ( - <DashboardComponent - key={componentId} - id={componentId} - parentId={columnComponent.id} - depth={depth + 1} - index={itemIndex} - availableColumnCount={columnComponent.meta.width} - columnWidth={columnWidth} - onResizeStart={onResizeStart} - onResize={onResize} - onResizeStop={onResizeStop} - isComponentVisible={isComponentVisible} - onChangeTab={onChangeTab} - /> + <React.Fragment key={componentId}> + <DashboardComponent + id={componentId} + parentId={columnComponent.id} + depth={depth + 1} + index={itemIndex} + availableColumnCount={columnComponent.meta.width} + columnWidth={columnWidth} + onResizeStart={onResizeStart} + onResize={onResize} + onResizeStop={onResizeStop} + isComponentVisible={isComponentVisible} + onChangeTab={onChangeTab} + /> + {editMode && ( + <Droppable + component={columnItems} + parentComponent={columnComponent} + depth={depth + 1} + index={itemIndex + 1} + orientation="column" + onDrop={handleComponentDrop} + className={cx( + 'empty-droptarget', + itemIndex === columnItems.length - 1 && + 'droptarget-edge', + )} + editMode + > + {({ dropIndicatorProps }) => + dropIndicatorProps && ( + <div {...dropIndicatorProps} /> + ) + } + </Droppable> + )} + </React.Fragment> )) )} - - {dropIndicatorProps && <div {...dropIndicatorProps} />} </ColumnStyles> </WithPopoverMenu> </ResizableContainer> )} - </DragDroppable> + </Draggable> ); } } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Column.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Column.test.jsx index 2d759b812a..b1521211ad 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Column.test.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Column.test.jsx @@ -27,6 +27,14 @@ import { getMockStore } from 'spec/fixtures/mockStore'; import { dashboardLayout as mockLayout } from 'spec/fixtures/mockDashboardLayout'; import { initialState } from 'src/SqlLab/fixtures'; +jest.mock('src/dashboard/components/dnd/DragDroppable', () => ({ + Draggable: ({ children }) => ( + <div data-test="mock-draggable">{children({})}</div> + ), + Droppable: ({ children }) => ( + <div data-test="mock-droppable">{children({})}</div> + ), +})); jest.mock( 'src/dashboard/containers/DashboardComponent', () => @@ -92,10 +100,12 @@ function setup(overrideProps) { }); } -test('should render a DragDroppable', () => { - // don't count child DragDroppables - const { getByTestId } = setup({ component: columnWithoutChildren }); - expect(getByTestId('dragdroppable-object')).toBeInTheDocument(); +test('should render a Draggable', () => { + const { getByTestId, queryByTestId } = setup({ + component: columnWithoutChildren, + }); + expect(getByTestId('mock-draggable')).toBeInTheDocument(); + expect(queryByTestId('mock-droppable')).not.toBeInTheDocument(); }); test('should skip rendering HoverMenu and DeleteComponentButton when not in editMode', () => { @@ -120,11 +130,14 @@ test('should render a ResizableContainer', () => { test('should render a HoverMenu in editMode', () => { // we cannot set props on the Row because of the WithDragDropContext wrapper - const { container } = setup({ + const { container, getAllByTestId } = setup({ component: columnWithoutChildren, editMode: true, }); expect(container.querySelector('.hover-menu')).toBeInTheDocument(); + + // Droppable area enabled in editMode + expect(getAllByTestId('mock-droppable').length).toBe(1); }); test('should render a DeleteComponentButton in editMode', () => { diff --git a/superset-frontend/src/dashboard/components/gridComponents/Divider.jsx b/superset-frontend/src/dashboard/components/gridComponents/Divider.jsx index 078405be3e..638527aa3b 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Divider.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Divider.jsx @@ -20,7 +20,7 @@ import React from 'react'; import PropTypes from 'prop-types'; import { css, styled } from '@superset-ui/core'; -import DragDroppable from '../dnd/DragDroppable'; +import { Draggable } from '../dnd/DragDroppable'; import HoverMenu from '../menu/HoverMenu'; import DeleteComponentButton from '../DeleteComponentButton'; import { componentShape } from '../../util/propShapes'; @@ -84,7 +84,7 @@ class Divider extends React.PureComponent { } = this.props; return ( - <DragDroppable + <Draggable component={component} parentComponent={parentComponent} orientation="row" @@ -93,20 +93,17 @@ class Divider extends React.PureComponent { onDrop={handleComponentDrop} editMode={editMode} > - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => ( <div ref={dragSourceRef}> {editMode && ( <HoverMenu position="left"> <DeleteComponentButton onDelete={this.handleDeleteComponent} /> </HoverMenu> )} - <DividerLine className="dashboard-component dashboard-component-divider" /> - - {dropIndicatorProps && <div {...dropIndicatorProps} />} </div> )} - </DragDroppable> + </Draggable> ); } } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Divider.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Divider.test.jsx index 6331f68832..f98378ab72 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Divider.test.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Divider.test.jsx @@ -24,7 +24,7 @@ import { HTML5Backend } from 'react-dnd-html5-backend'; import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton'; import HoverMenu from 'src/dashboard/components/menu/HoverMenu'; -import DragDroppable from 'src/dashboard/components/dnd/DragDroppable'; +import { Draggable } from 'src/dashboard/components/dnd/DragDroppable'; import Divider from 'src/dashboard/components/gridComponents/Divider'; import newComponentFactory from 'src/dashboard/util/newComponentFactory'; import { @@ -56,9 +56,9 @@ describe('Divider', () => { return wrapper; } - it('should render a DragDroppable', () => { + it('should render a Draggable', () => { const wrapper = setup(); - expect(wrapper.find(DragDroppable)).toExist(); + expect(wrapper.find(Draggable)).toExist(); }); it('should render a div with class "dashboard-component-divider"', () => { diff --git a/superset-frontend/src/dashboard/components/gridComponents/DynamicComponent.tsx b/superset-frontend/src/dashboard/components/gridComponents/DynamicComponent.tsx index 707597d8c0..f1752588d2 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/DynamicComponent.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/DynamicComponent.tsx @@ -21,7 +21,7 @@ import { DashboardComponentMetadata, JsonObject, t } from '@superset-ui/core'; import backgroundStyleOptions from 'src/dashboard/util/backgroundStyleOptions'; import cx from 'classnames'; import { useSelector } from 'react-redux'; -import DragDroppable from '../dnd/DragDroppable'; +import { Draggable } from '../dnd/DragDroppable'; import { COLUMN_TYPE, ROW_TYPE } from '../../util/componentTypes'; import WithPopoverMenu from '../menu/WithPopoverMenu'; import ResizableContainer from '../resizable/ResizableContainer'; @@ -106,7 +106,7 @@ const DynamicComponent: FC<FilterSummaryType> = ({ ); return ( - <DragDroppable + <Draggable // @ts-ignore component={component} // @ts-ignore @@ -117,7 +117,7 @@ const DynamicComponent: FC<FilterSummaryType> = ({ onDrop={handleComponentDrop} editMode={editMode} > - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => ( <WithPopoverMenu menuItems={[ <BackgroundStyleDropdown @@ -168,10 +168,9 @@ const DynamicComponent: FC<FilterSummaryType> = ({ </div> </ResizableContainer> </div> - {dropIndicatorProps && <div {...dropIndicatorProps} />} </WithPopoverMenu> )} - </DragDroppable> + </Draggable> ); }; export default DynamicComponent; diff --git a/superset-frontend/src/dashboard/components/gridComponents/Header.jsx b/superset-frontend/src/dashboard/components/gridComponents/Header.jsx index 253f377fc2..85f4cd7cc7 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Header.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Header.jsx @@ -23,7 +23,7 @@ import { css, styled } from '@superset-ui/core'; import PopoverDropdown from 'src/components/PopoverDropdown'; import EditableTitle from 'src/components/EditableTitle'; -import DragDroppable from 'src/dashboard/components/dnd/DragDroppable'; +import { Draggable } from 'src/dashboard/components/dnd/DragDroppable'; import DragHandle from 'src/dashboard/components/dnd/DragHandle'; import AnchorLink from 'src/dashboard/components/AnchorLink'; import HoverMenu from 'src/dashboard/components/menu/HoverMenu'; @@ -178,7 +178,7 @@ class Header extends React.PureComponent { ); return ( - <DragDroppable + <Draggable component={component} parentComponent={parentComponent} orientation="row" @@ -188,7 +188,7 @@ class Header extends React.PureComponent { disableDragDrop={isFocused} editMode={editMode} > - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => ( <div ref={dragSourceRef}> {editMode && depth <= 2 && ( // drag handle looks bad when nested @@ -239,11 +239,9 @@ class Header extends React.PureComponent { )} </HeaderStyles> </WithPopoverMenu> - - {dropIndicatorProps && <div {...dropIndicatorProps} />} </div> )} - </DragDroppable> + </Draggable> ); } } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Header.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Header.test.jsx index 6f903a05c1..64ff712129 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Header.test.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Header.test.jsx @@ -27,7 +27,7 @@ import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButto import EditableTitle from 'src/components/EditableTitle'; import HoverMenu from 'src/dashboard/components/menu/HoverMenu'; import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu'; -import DragDroppable from 'src/dashboard/components/dnd/DragDroppable'; +import { Draggable } from 'src/dashboard/components/dnd/DragDroppable'; import Header from 'src/dashboard/components/gridComponents/Header'; import newComponentFactory from 'src/dashboard/util/newComponentFactory'; import { @@ -65,9 +65,9 @@ describe('Header', () => { return wrapper; } - it('should render a DragDroppable', () => { + it('should render a Draggable', () => { const wrapper = setup(); - expect(wrapper.find(DragDroppable)).toExist(); + expect(wrapper.find(Draggable)).toExist(); }); it('should render a WithPopoverMenu', () => { diff --git a/superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx b/superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx index 9febfacf90..23a06a0f7d 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx @@ -26,7 +26,7 @@ import { Logger, LOG_ACTIONS_RENDER_CHART } from 'src/logger/LogUtils'; import { MarkdownEditor } from 'src/components/AsyncAceEditor'; import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton'; -import DragDroppable from 'src/dashboard/components/dnd/DragDroppable'; +import { Draggable } from 'src/dashboard/components/dnd/DragDroppable'; import HoverMenu from 'src/dashboard/components/menu/HoverMenu'; import ResizableContainer from 'src/dashboard/components/resizable/ResizableContainer'; import MarkdownModeDropdown from 'src/dashboard/components/menu/MarkdownModeDropdown'; @@ -332,7 +332,7 @@ class Markdown extends React.PureComponent { const isEditing = editorMode === 'edit'; return ( - <DragDroppable + <Draggable component={component} parentComponent={parentComponent} orientation={parentComponent.type === ROW_TYPE ? 'column' : 'row'} @@ -342,7 +342,7 @@ class Markdown extends React.PureComponent { disableDragDrop={isFocused} editMode={editMode} > - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => ( <WithPopoverMenu onChangeFocus={this.handleChangeFocus} menuItems={[ @@ -396,10 +396,9 @@ class Markdown extends React.PureComponent { </div> </ResizableContainer> </MarkdownStyles> - {dropIndicatorProps && <div {...dropIndicatorProps} />} </WithPopoverMenu> )} - </DragDroppable> + </Draggable> ); } } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Markdown.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Markdown.test.jsx index ac97c38a4a..b15c4c504f 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Markdown.test.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Markdown.test.jsx @@ -30,7 +30,7 @@ import MarkdownConnected from 'src/dashboard/components/gridComponents/Markdown' import MarkdownModeDropdown from 'src/dashboard/components/menu/MarkdownModeDropdown'; import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton'; import waitForComponentToPaint from 'spec/helpers/waitForComponentToPaint'; -import DragDroppable from 'src/dashboard/components/dnd/DragDroppable'; +import { Draggable } from 'src/dashboard/components/dnd/DragDroppable'; import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu'; import ResizableContainer from 'src/dashboard/components/resizable/ResizableContainer'; @@ -62,7 +62,7 @@ describe('Markdown', () => { function setup(overrideProps) { // We have to wrap provide DragDropContext for the underlying DragDroppable - // otherwise we cannot assert on DragDroppable children + // otherwise we cannot assert on Droppable children const wrapper = mount( <Provider store={mockStore}> <DndProvider backend={HTML5Backend}> @@ -73,9 +73,9 @@ describe('Markdown', () => { return wrapper; } - it('should render a DragDroppable', () => { + it('should render a Draggable', () => { const wrapper = setup(); - expect(wrapper.find(DragDroppable)).toExist(); + expect(wrapper.find(Draggable)).toExist(); }); it('should render a WithPopoverMenu', () => { diff --git a/superset-frontend/src/dashboard/components/gridComponents/Row.jsx b/superset-frontend/src/dashboard/components/gridComponents/Row.jsx index d645a1a2cf..95560e3b59 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Row.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Row.jsx @@ -19,15 +19,20 @@ import React from 'react'; import PropTypes from 'prop-types'; import cx from 'classnames'; +import { debounce } from 'lodash'; import { css, + FAST_DEBOUNCE, FeatureFlag, isFeatureEnabled, styled, t, } from '@superset-ui/core'; -import DragDroppable from 'src/dashboard/components/dnd/DragDroppable'; +import { + Draggable, + Droppable, +} from 'src/dashboard/components/dnd/DragDroppable'; import DragHandle from 'src/dashboard/components/dnd/DragHandle'; import DashboardComponent from 'src/dashboard/containers/DashboardComponent'; import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton'; @@ -39,6 +44,7 @@ import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu'; import { componentShape } from 'src/dashboard/util/propShapes'; import backgroundStyleOptions from 'src/dashboard/util/backgroundStyleOptions'; import { BACKGROUND_TRANSPARENT } from 'src/dashboard/util/constants'; +import { EMPTY_CONTAINER_Z_INDEX } from 'src/dashboard/constants'; import { isCurrentUserBot } from 'src/utils/isBot'; const propTypes = { @@ -57,6 +63,7 @@ const propTypes = { onResizeStart: PropTypes.func.isRequired, onResize: PropTypes.func.isRequired, onResizeStop: PropTypes.func.isRequired, + maxChildrenHeight: PropTypes.number.isRequired, // dnd handleComponentDrop: PropTypes.func.isRequired, @@ -65,7 +72,7 @@ const propTypes = { }; const GridRow = styled.div` - ${({ theme }) => css` + ${({ theme, editMode }) => css` position: relative; display: flex; flex-direction: row; @@ -75,7 +82,35 @@ const GridRow = styled.div` height: fit-content; & > :not(:last-child):not(.hover-menu) { - margin-right: ${theme.gridUnit * 4}px; + ${!editMode && `margin-right: ${theme.gridUnit * 4}px;`} + } + + & .empty-droptarget { + position: relative; + align-self: center; + &.empty-droptarget--vertical { + min-width: ${theme.gridUnit * 4}px; + &:not(:last-child) { + width: ${theme.gridUnit * 4}px; + } + &:first-child:not(.droptarget-side) { + z-index: ${EMPTY_CONTAINER_Z_INDEX}; + position: absolute; + width: 100%; + height: 100%; + } + } + &.droptarget-side { + z-index: ${EMPTY_CONTAINER_Z_INDEX}; + position: absolute; + width: ${theme.gridUnit * 4}px; + &:first-child { + inset-inline-start: 0; + } + &:last-child { + inset-inline-end: 0; + } + } } &.grid-row--empty { @@ -108,6 +143,10 @@ class Row extends React.PureComponent { 'background', ); this.handleChangeFocus = this.handleChangeFocus.bind(this); + this.setVerticalEmptyContainerHeight = debounce( + this.setVerticalEmptyContainerHeight.bind(this), + FAST_DEBOUNCE, + ); this.containerRef = React.createRef(); this.observerEnabler = null; @@ -145,10 +184,28 @@ class Row extends React.PureComponent { if (element) { this.observerEnabler.observe(element); this.observerDisabler.observe(element); + this.setVerticalEmptyContainerHeight(); } } } + componentDidUpdate() { + this.setVerticalEmptyContainerHeight(); + } + + setVerticalEmptyContainerHeight() { + const { containerHeight } = this.state; + const { editMode } = this.props; + const updatedHeight = this.containerRef.current?.clientHeight; + if ( + editMode && + this.containerRef.current && + updatedHeight !== containerHeight + ) { + this.setState({ containerHeight: updatedHeight }); + } + } + componentWillUnmount() { this.observerEnabler?.disconnect(); this.observerDisabler?.disconnect(); @@ -195,6 +252,7 @@ class Row extends React.PureComponent { onChangeTab, isComponentVisible, } = this.props; + const { containerHeight } = this.state; const rowItems = rowComponent.children || []; @@ -202,9 +260,10 @@ class Row extends React.PureComponent { opt => opt.value === (rowComponent.meta.background || BACKGROUND_TRANSPARENT), ); + const remainColumnCount = availableColumnCount - occupiedColumnCount; return ( - <DragDroppable + <Draggable component={rowComponent} parentComponent={parentComponent} orientation="row" @@ -213,7 +272,7 @@ class Row extends React.PureComponent { onDrop={handleComponentDrop} editMode={editMode} > - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => ( <WithPopoverMenu isFocused={this.state.isFocused} onChangeFocus={this.handleChangeFocus} @@ -245,36 +304,94 @@ class Row extends React.PureComponent { )} data-test={`grid-row-${backgroundStyle.className}`} ref={this.containerRef} + editMode={editMode} > - {rowItems.length === 0 ? ( + {editMode && ( + <Droppable + {...(rowItems.length === 0 + ? { + component: rowComponent, + parentComponent, + dropToChild: true, + } + : { + component: rowItems, + parentComponent: rowComponent, + })} + depth={depth + 1} + index={0} + orientation="row" + onDrop={handleComponentDrop} + className={cx( + 'empty-droptarget', + 'empty-droptarget--vertical', + rowItems.length > 0 && 'droptarget-side', + )} + editMode + style={{ + height: rowItems.length > 0 ? containerHeight : '100%', + ...(rowItems.length > 0 && { width: 16 }), + }} + > + {({ dropIndicatorProps }) => + dropIndicatorProps && <div {...dropIndicatorProps} /> + } + </Droppable> + )} + {rowItems.length === 0 && ( <div css={emptyRowContentStyles}>{t('Empty row')}</div> - ) : ( - rowItems.map((componentId, itemIndex) => ( - <DashboardComponent - key={componentId} - id={componentId} - parentId={rowComponent.id} - depth={depth + 1} - index={itemIndex} - availableColumnCount={ - availableColumnCount - occupiedColumnCount - } - columnWidth={columnWidth} - onResizeStart={onResizeStart} - onResize={onResize} - onResizeStop={onResizeStop} - isComponentVisible={isComponentVisible} - onChangeTab={onChangeTab} - isInView={this.state.isInView} - /> - )) )} - - {dropIndicatorProps && <div {...dropIndicatorProps} />} + {rowItems.length > 0 && + rowItems.map((componentId, itemIndex) => ( + <React.Fragment key={componentId}> + <DashboardComponent + key={componentId} + id={componentId} + parentId={rowComponent.id} + depth={depth + 1} + index={itemIndex} + availableColumnCount={remainColumnCount} + columnWidth={columnWidth} + onResizeStart={onResizeStart} + onResize={onResize} + onResizeStop={onResizeStop} + isComponentVisible={isComponentVisible} + onChangeTab={onChangeTab} + isInView={this.state.isInView} + /> + {editMode && ( + <Droppable + component={rowItems} + parentComponent={rowComponent} + depth={depth + 1} + index={itemIndex + 1} + orientation="row" + onDrop={handleComponentDrop} + className={cx( + 'empty-droptarget', + 'empty-droptarget--vertical', + remainColumnCount === 0 && + itemIndex === rowItems.length - 1 && + 'droptarget-side', + )} + editMode + style={{ + height: containerHeight, + ...(remainColumnCount === 0 && + itemIndex === rowItems.length - 1 && { width: 16 }), + }} + > + {({ dropIndicatorProps }) => + dropIndicatorProps && <div {...dropIndicatorProps} /> + } + </Droppable> + )} + </React.Fragment> + ))} </GridRow> </WithPopoverMenu> )} - </DragDroppable> + </Draggable> ); } } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Row.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Row.test.jsx index b69a8f7e97..e330195d77 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Row.test.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Row.test.jsx @@ -28,6 +28,14 @@ import { getMockStore } from 'spec/fixtures/mockStore'; import { dashboardLayout as mockLayout } from 'spec/fixtures/mockDashboardLayout'; import { initialState } from 'src/SqlLab/fixtures'; +jest.mock('src/dashboard/components/dnd/DragDroppable', () => ({ + Draggable: ({ children }) => ( + <div data-test="mock-draggable">{children({})}</div> + ), + Droppable: ({ children }) => ( + <div data-test="mock-droppable">{children({})}</div> + ), +})); jest.mock( 'src/dashboard/containers/DashboardComponent', () => @@ -92,10 +100,14 @@ function setup(overrideProps) { }); } -test('should render a DragDroppable', () => { +test('should render a Draggable', () => { // don't count child DragDroppables - const { getByTestId } = setup({ component: rowWithoutChildren }); - expect(getByTestId('dragdroppable-object')).toBeInTheDocument(); + const { getByTestId, queryByTestId } = setup({ + component: rowWithoutChildren, + }); + + expect(getByTestId('mock-draggable')).toBeInTheDocument(); + expect(queryByTestId('mock-droppable')).not.toBeInTheDocument(); }); test('should skip rendering HoverMenu and DeleteComponentButton when not in editMode', () => { @@ -113,11 +125,14 @@ test('should render a WithPopoverMenu', () => { }); test('should render a HoverMenu in editMode', () => { - const { container } = setup({ + const { container, getAllByTestId } = setup({ component: rowWithoutChildren, editMode: true, }); expect(container.querySelector('.hover-menu')).toBeInTheDocument(); + + // Droppable area enabled in editMode + expect(getAllByTestId('mock-droppable').length).toBe(1); }); test('should render a DeleteComponentButton in editMode', () => { diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tab.jsx b/superset-frontend/src/dashboard/components/gridComponents/Tab.jsx index d1d08176ba..b546947e31 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tab.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tab.jsx @@ -28,7 +28,9 @@ import EditableTitle from 'src/components/EditableTitle'; import { setEditMode } from 'src/dashboard/actions/dashboardState'; import DashboardComponent from 'src/dashboard/containers/DashboardComponent'; import AnchorLink from 'src/dashboard/components/AnchorLink'; -import DragDroppable from 'src/dashboard/components/dnd/DragDroppable'; +import DragDroppable, { + Droppable, +} from 'src/dashboard/components/dnd/DragDroppable'; import { componentShape } from 'src/dashboard/util/propShapes'; export const RENDER_TAB = 'RENDER_TAB'; @@ -83,15 +85,8 @@ 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" /> - ); +const renderDraggableContent = dropProps => + dropProps.dropIndicatorProps && <div {...dropProps.dropIndicatorProps} />; class Tab extends React.PureComponent { constructor(props) { @@ -147,7 +142,6 @@ class Tab extends React.PureComponent { renderTabContent() { const { component: tabComponent, - parentComponent: tabParentComponent, depth, availableColumnCount, columnWidth, @@ -166,21 +160,25 @@ class Tab extends React.PureComponent { <div className="dashboard-component-tabs-content"> {/* Make top of tab droppable */} {editMode && ( - <DragDroppable + <Droppable component={tabComponent} - parentComponent={tabParentComponent} orientation="column" index={0} depth={depth} - onDrop={this.handleTopDropTargetDrop} + onDrop={ + tabComponent.children.length === 0 + ? this.handleTopDropTargetDrop + : this.handleDrop + } editMode className={classNames({ 'empty-droptarget': true, 'empty-droptarget--full': tabComponent.children.length === 0, })} + dropToChild={tabComponent.children.length === 0} > - {renderDraggableContentTop} - </DragDroppable> + {renderDraggableContent} + </Droppable> )} {shouldDisplayEmptyState && ( <EmptyStateMedium @@ -220,39 +218,38 @@ class Tab extends React.PureComponent { /> )} {tabComponent.children.map((componentId, componentIndex) => ( - <DashboardComponent - key={componentId} - id={componentId} - parentId={tabComponent.id} - depth={depth} // see isValidChild.js for why tabs don't increment child depth - index={componentIndex} - onDrop={this.handleDrop} - onHover={this.handleOnHover} - availableColumnCount={availableColumnCount} - columnWidth={columnWidth} - onResizeStart={onResizeStart} - onResize={onResize} - onResizeStop={onResizeStop} - isComponentVisible={isComponentVisible} - onChangeTab={this.handleChangeTab} - /> + <React.Fragment key={componentId}> + <DashboardComponent + id={componentId} + parentId={tabComponent.id} + depth={depth} // see isValidChild.js for why tabs don't increment child depth + index={componentIndex} + onDrop={this.handleDrop} + onHover={this.handleOnHover} + availableColumnCount={availableColumnCount} + columnWidth={columnWidth} + onResizeStart={onResizeStart} + onResize={onResize} + onResizeStop={onResizeStop} + isComponentVisible={isComponentVisible} + onChangeTab={this.handleChangeTab} + /> + {/* Make bottom of tab droppable */} + {editMode && ( + <Droppable + component={tabComponent} + orientation="column" + index={componentIndex + 1} + depth={depth} + onDrop={this.handleDrop} + editMode + className="empty-droptarget" + > + {renderDraggableContent} + </Droppable> + )} + </React.Fragment> ))} - {/* Make bottom of tab droppable */} - {editMode && tabComponent.children.length > 0 && ( - <DragDroppable - component={tabComponent} - parentComponent={tabParentComponent} - orientation="column" - index={tabComponent.children.length} - depth={depth} - onDrop={this.handleDrop} - onHover={this.handleOnHover} - editMode - className="empty-droptarget" - > - {renderDraggableContentBottom} - </DragDroppable> - )} </div> ); } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tab.test.tsx b/superset-frontend/src/dashboard/components/gridComponents/Tab.test.tsx index d995595c49..e19a086d81 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tab.test.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tab.test.tsx @@ -22,7 +22,6 @@ import React from 'react'; import { render, screen } from 'spec/helpers/testing-library'; import DashboardComponent from 'src/dashboard/containers/DashboardComponent'; import EditableTitle from 'src/components/EditableTitle'; -import DragDroppable from 'src/dashboard/components/dnd/DragDroppable'; import { setEditMode } from 'src/dashboard/actions/dashboardState'; import Tab from './Tab'; @@ -37,8 +36,9 @@ jest.mock('src/components/EditableTitle', () => </button> )), ); -jest.mock('src/dashboard/components/dnd/DragDroppable', () => - jest.fn(props => { +jest.mock('src/dashboard/components/dnd/DragDroppable', () => ({ + ...jest.requireActual('src/dashboard/components/dnd/DragDroppable'), + Droppable: jest.fn(props => { const childProps = props.editMode ? { dragSourceRef: props.dragSourceRef, @@ -47,14 +47,14 @@ jest.mock('src/dashboard/components/dnd/DragDroppable', () => : {}; return ( <div> - <button type="button" data-test="DragDroppable" onClick={props.onDrop}> + <button type="button" data-test="MockDroppable" onClick={props.onDrop}> DragDroppable </button> {props.children(childProps)} </div> ); }), -); +})); jest.mock('src/dashboard/actions/dashboardState', () => ({ setEditMode: jest.fn(() => ({ type: 'SET_EDIT_MODE', @@ -106,30 +106,39 @@ beforeEach(() => { test('Render tab (no content)', () => { const props = createProps(); props.renderType = 'RENDER_TAB'; - render(<Tab {...props} />, { useRedux: true, useDnd: true }); + const { getByTestId } = render(<Tab {...props} />, { + useRedux: true, + useDnd: true, + }); expect(screen.getByText('🚀 Aspiring Developers')).toBeInTheDocument(); expect(EditableTitle).toBeCalledTimes(1); - expect(DragDroppable).toBeCalledTimes(1); + expect(getByTestId('dragdroppable-object')).toBeInTheDocument(); }); test('Render tab (no content) editMode:true', () => { const props = createProps(); props.editMode = true; props.renderType = 'RENDER_TAB'; - render(<Tab {...props} />, { useRedux: true, useDnd: true }); + const { getByTestId } = render(<Tab {...props} />, { + useRedux: true, + useDnd: true, + }); expect(screen.getByText('🚀 Aspiring Developers')).toBeInTheDocument(); expect(EditableTitle).toBeCalledTimes(1); - expect(DragDroppable).toBeCalledTimes(1); + expect(getByTestId('dragdroppable-object')).toBeInTheDocument(); }); test('Edit table title', () => { const props = createProps(); props.editMode = true; props.renderType = 'RENDER_TAB'; - render(<Tab {...props} />, { useRedux: true, useDnd: true }); + const { getByTestId } = render(<Tab {...props} />, { + useRedux: true, + useDnd: true, + }); expect(EditableTitle).toBeCalledTimes(1); - expect(DragDroppable).toBeCalledTimes(1); + expect(getByTestId('dragdroppable-object')).toBeInTheDocument(); expect(props.updateComponents).not.toBeCalled(); userEvent.click(screen.getByText('🚀 Aspiring Developers')); @@ -139,7 +148,10 @@ test('Edit table title', () => { test('Render tab (with content)', () => { const props = createProps(); props.isFocused = true; - render(<Tab {...props} />, { useRedux: true, useDnd: true }); + const { queryByTestId } = render(<Tab {...props} />, { + useRedux: true, + useDnd: true, + }); expect(DashboardComponent).toBeCalledTimes(2); expect(DashboardComponent).toHaveBeenNthCalledWith( 1, @@ -177,7 +189,7 @@ test('Render tab (with content)', () => { }), {}, ); - expect(DragDroppable).toBeCalledTimes(0); + expect(queryByTestId('dragdroppable-object')).not.toBeInTheDocument(); }); test('Render tab content with no children', () => { @@ -215,7 +227,10 @@ test('Render tab (with content) editMode:true', () => { const props = createProps(); props.isFocused = true; props.editMode = true; - render(<Tab {...props} />, { useRedux: true, useDnd: true }); + const { getAllByTestId } = render(<Tab {...props} />, { + useRedux: true, + useDnd: true, + }); expect(DashboardComponent).toBeCalledTimes(2); expect(DashboardComponent).toHaveBeenNthCalledWith( 1, @@ -253,20 +268,28 @@ test('Render tab (with content) editMode:true', () => { }), {}, ); - expect(DragDroppable).toBeCalledTimes(2); + // 3 droppable area exists for two child components + expect(getAllByTestId('MockDroppable')).toHaveLength(3); }); test('Should call "handleDrop" and "handleTopDropTargetDrop"', () => { const props = createProps(); props.isFocused = true; props.editMode = true; - render(<Tab {...props} />, { useRedux: true, useDnd: true }); + const { getAllByTestId, rerender } = render( + <Tab {...props} component={{ ...props.component, children: [] }} />, + { + useRedux: true, + useDnd: true, + }, + ); expect(props.handleComponentDrop).not.toBeCalled(); - userEvent.click(screen.getAllByRole('button')[0]); + userEvent.click(getAllByTestId('MockDroppable')[0]); expect(props.handleComponentDrop).toBeCalledTimes(1); expect(props.onDropOnTab).not.toBeCalled(); - userEvent.click(screen.getAllByRole('button')[1]); + rerender(<Tab {...props} />); + userEvent.click(getAllByTestId('MockDroppable')[1]); expect(props.onDropOnTab).toBeCalledTimes(1); expect(props.handleComponentDrop).toBeCalledTimes(2); }); diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx b/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx index 67f4b3c598..c9f35d3ba5 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx @@ -23,7 +23,7 @@ import { connect } from 'react-redux'; import { LineEditableTabs } from 'src/components/Tabs'; import { LOG_ACTIONS_SELECT_DASHBOARD_TAB } from 'src/logger/LogUtils'; import { AntdModal } from 'src/components'; -import DragDroppable from '../dnd/DragDroppable'; +import { Draggable } from '../dnd/DragDroppable'; import DragHandle from '../dnd/DragHandle'; import DashboardComponent from '../../containers/DashboardComponent'; import DeleteComponentButton from '../DeleteComponentButton'; @@ -32,7 +32,7 @@ import findTabIndexByComponentId from '../../util/findTabIndexByComponentId'; import getDirectPathToTabIndex from '../../util/getDirectPathToTabIndex'; import getLeafComponentIdFromPath from '../../util/getLeafComponentIdFromPath'; import { componentShape } from '../../util/propShapes'; -import { NEW_TAB_ID, DASHBOARD_ROOT_ID } from '../../util/constants'; +import { NEW_TAB_ID } from '../../util/constants'; import { RENDER_TAB, RENDER_TAB_CONTENT } from './Tab'; import { TABS_TYPE, TAB_TYPE } from '../../util/componentTypes'; @@ -339,7 +339,7 @@ export class Tabs extends React.PureComponent { tabsToHighlight = nativeFilters.filters[highlightedFilterId]?.tabsInScope; } return ( - <DragDroppable + <Draggable component={tabsComponent} parentComponent={parentComponent} orientation="row" @@ -348,10 +348,7 @@ export class Tabs extends React.PureComponent { onDrop={this.handleDrop} editMode={editMode} > - {({ - dropIndicatorProps: tabsDropIndicatorProps, - dragSourceRef: tabsDragSourceRef, - }) => ( + {({ dragSourceRef: tabsDragSourceRef }) => ( <StyledTabsContainer className="dashboard-component dashboard-component-tabs" data-test="dashboard-component-tabs" @@ -415,15 +412,9 @@ export class Tabs extends React.PureComponent { </LineEditableTabs.TabPane> ))} </LineEditableTabs> - - {/* don't indicate that a drop on root is allowed when tabs already exist */} - {tabsDropIndicatorProps && - parentComponent.id !== DASHBOARD_ROOT_ID && ( - <div {...tabsDropIndicatorProps} /> - )} </StyledTabsContainer> )} - </DragDroppable> + </Draggable> ); } } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tabs.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Tabs.test.jsx index 127b4d42db..7f39bdcc68 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tabs.test.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tabs.test.jsx @@ -29,6 +29,14 @@ import { getMockStore } from 'spec/fixtures/mockStore'; import { nativeFilters } from 'spec/fixtures/mockNativeFilters'; import { initialState } from 'src/SqlLab/fixtures'; +jest.mock('src/dashboard/components/dnd/DragDroppable', () => ({ + Draggable: ({ children }) => ( + <div data-test="mock-draggable">{children({})}</div> + ), + Droppable: ({ children }) => ( + <div data-test="mock-droppable">{children({})}</div> + ), +})); jest.mock('src/dashboard/containers/DashboardComponent', () => ({ id }) => ( <div data-test="mock-dashboard-component">{id}</div> )); @@ -88,12 +96,12 @@ function setup(overrideProps) { }); } -test('should render a DragDroppable', () => { - // test just Tabs with no children DragDroppables +test('should render a Draggable', () => { + // test just Tabs with no children Draggable const { getByTestId } = setup({ component: { ...props.component, children: [] }, }); - expect(getByTestId('dragdroppable-object')).toBeInTheDocument(); + expect(getByTestId('mock-draggable')).toBeInTheDocument(); }); test('should render non-editable tabs', () => { diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tabs.test.tsx b/superset-frontend/src/dashboard/components/gridComponents/Tabs.test.tsx index 9ee9fc6866..6aef193b44 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tabs.test.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tabs.test.tsx @@ -22,7 +22,7 @@ import React from 'react'; import { render, screen, waitFor } from 'spec/helpers/testing-library'; import { nativeFiltersInfo } from 'src/dashboard/fixtures/mockNativeFilters'; import DashboardComponent from 'src/dashboard/containers/DashboardComponent'; -import DragDroppable from 'src/dashboard/components/dnd/DragDroppable'; +import { Draggable } from 'src/dashboard/components/dnd/DragDroppable'; import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton'; import getLeafComponentIdFromPath from 'src/dashboard/util/getLeafComponentIdFromPath'; import emptyDashboardLayout from 'src/dashboard/fixtures/emptyDashboardLayout'; @@ -55,8 +55,8 @@ jest.mock('src/dashboard/components/DeleteComponentButton', () => ); jest.mock('src/dashboard/util/getLeafComponentIdFromPath', () => jest.fn()); -jest.mock('src/dashboard/components/dnd/DragDroppable', () => - jest.fn(props => { +jest.mock('src/dashboard/components/dnd/DragDroppable', () => ({ + Draggable: jest.fn(props => { const childProps = props.editMode ? { dragSourceRef: props.dragSourceRef, @@ -72,7 +72,7 @@ jest.mock('src/dashboard/components/dnd/DragDroppable', () => </div> ); }), -); +})); const createProps = () => ({ id: 'TABS-L-d9eyOE-b', @@ -123,7 +123,7 @@ test('Should render editMode:true', () => { const props = createProps(); render(<Tabs {...props} />, { useRedux: true, useDnd: true }); expect(screen.getAllByRole('tab')).toHaveLength(3); - expect(DragDroppable).toBeCalledTimes(1); + expect(Draggable).toBeCalledTimes(1); expect(DashboardComponent).toBeCalledTimes(4); expect(DeleteComponentButton).toBeCalledTimes(1); expect(screen.getAllByRole('button', { name: 'remove' })).toHaveLength(3); @@ -135,7 +135,7 @@ test('Should render editMode:false', () => { props.editMode = false; render(<Tabs {...props} />, { useRedux: true, useDnd: true }); expect(screen.getAllByRole('tab')).toHaveLength(3); - expect(DragDroppable).toBeCalledTimes(1); + expect(Draggable).toBeCalledTimes(1); expect(DashboardComponent).toBeCalledTimes(4); expect(DeleteComponentButton).not.toBeCalled(); expect( diff --git a/superset-frontend/src/dashboard/constants.ts b/superset-frontend/src/dashboard/constants.ts index d4e5fed4b6..d512c2a8a6 100644 --- a/superset-frontend/src/dashboard/constants.ts +++ b/superset-frontend/src/dashboard/constants.ts @@ -43,6 +43,7 @@ export const FILTER_BAR_HEADER_HEIGHT = 80; export const FILTER_BAR_TABS_HEIGHT = 46; export const BUILDER_SIDEPANEL_WIDTH = 374; export const OVERWRITE_INSPECT_FIELDS = ['css', 'json_metadata.filter_scopes']; +export const EMPTY_CONTAINER_Z_INDEX = 10; export const DEFAULT_CROSS_FILTER_SCOPING: NativeFilterScope = { rootPath: [DASHBOARD_ROOT_ID], diff --git a/superset-frontend/src/dashboard/util/getDropPosition.js b/superset-frontend/src/dashboard/util/getDropPosition.js index 81cbc48f46..c4a21ad113 100644 --- a/superset-frontend/src/dashboard/util/getDropPosition.js +++ b/superset-frontend/src/dashboard/util/getDropPosition.js @@ -23,6 +23,7 @@ export const DROP_TOP = 'DROP_TOP'; export const DROP_RIGHT = 'DROP_RIGHT'; export const DROP_BOTTOM = 'DROP_BOTTOM'; export const DROP_LEFT = 'DROP_LEFT'; +export const DROP_FORBIDDEN = 'DROP_FORBIDDEN'; // this defines how close the mouse must be to the edge of a component to display // a sibling type drop indicator @@ -72,7 +73,7 @@ export default function getDropPosition(monitor, Component) { }); if (!validChild && !validSibling) { - return null; + return DROP_FORBIDDEN; } const hasChildren = (component.children || []).length > 0; diff --git a/superset-frontend/src/dashboard/util/getDropPosition.test.js b/superset-frontend/src/dashboard/util/getDropPosition.test.js index 71f506b1bd..9fdd02afa6 100644 --- a/superset-frontend/src/dashboard/util/getDropPosition.test.js +++ b/superset-frontend/src/dashboard/util/getDropPosition.test.js @@ -21,6 +21,7 @@ import getDropPosition, { DROP_RIGHT, DROP_BOTTOM, DROP_LEFT, + DROP_FORBIDDEN, } from 'src/dashboard/util/getDropPosition'; import { @@ -80,7 +81,7 @@ describe('getDropPosition', () => { } describe('invalid child + invalid sibling', () => { - it('should return null', () => { + it('should return DROP_FORBIDDEN', () => { const result = getDropPosition( // TAB is an invalid child + sibling of GRID > ROW ...getMocks({ @@ -89,7 +90,7 @@ describe('getDropPosition', () => { draggingType: TAB_TYPE, }), ); - expect(result).toBeNull(); + expect(result).toBe(DROP_FORBIDDEN); }); });
