From ac8c283df04b6c4cbc24a5ae625e05a8f2679802 Mon Sep 17 00:00:00 2001 From: "JUST.in DO IT" Date: Wed, 21 Feb 2024 11:29:34 -0800 Subject: [PATCH] fix(dashboard): drag and drop indicator UX (#26699) --- .../DashboardBuilder/DashboardBuilder.tsx | 53 +++--- .../DashboardWrapper.test.tsx | 18 +- .../DashboardBuilder/DashboardWrapper.tsx | 33 +++- .../dashboard/components/DashboardGrid.jsx | 115 +++++++----- .../components/dnd/DragDroppable.jsx | 56 ++---- .../dashboard/components/dnd/handleDrop.js | 32 ++-- .../components/gridComponents/ChartHolder.tsx | 9 +- .../components/gridComponents/Column.jsx | 118 +++++++++--- .../components/gridComponents/Column.test.jsx | 23 ++- .../components/gridComponents/Divider.jsx | 11 +- .../gridComponents/Divider.test.jsx | 6 +- .../gridComponents/DynamicComponent.tsx | 9 +- .../components/gridComponents/Header.jsx | 10 +- .../components/gridComponents/Header.test.jsx | 6 +- .../components/gridComponents/Markdown.jsx | 9 +- .../gridComponents/Markdown.test.jsx | 8 +- .../components/gridComponents/Row.jsx | 175 +++++++++++++++--- .../components/gridComponents/Row.test.jsx | 23 ++- .../components/gridComponents/Tab.jsx | 93 +++++----- .../components/gridComponents/Tab.test.tsx | 59 ++++-- .../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 +- .../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 2756e71359674..e86d924b2104f 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 = () => { )} {/* @ts-ignore */} - = () => { style={draggableStyle} > {renderDraggableContent} - + { + jest.useFakeTimers(); +}); + +afterAll(() => { + jest.useRealTimers(); +}); + test('should render children', () => { const { getByTestId } = render( @@ -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: Test label, 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 5bb193de1bd4a..2bcdc5b9cd308 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 = ({ 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 70cf65218f2c3..93444c74238a7 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 && ( -
- ); - -const renderDraggableContentTop = dropProps => - dropProps.dropIndicatorProps && ( -
- ); +const renderDraggableContent = dropProps => + 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 { )}
- + {/* make the area above components droppable */} {editMode && ( - - {renderDraggableContentTop} - + {renderDraggableContent} + )} {gridComponent?.children?.map((id, index) => ( - + + + {/* make the area below components droppable */} + {editMode && ( + + {renderDraggableContent} + + )} + ))} - {/* make the area below components droppable */} - {editMode && gridComponent?.children?.length > 0 && ( - - {renderDraggableContentBottom} - - )} {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 6a49f9887550f..185b4c08c6c7f 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 450a60867c4d0..6d50d39c45425 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 a93f3b0b8db63..bcc58d0691dab 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 = ({ }, []); return ( - = ({ disableDragDrop={false} editMode={editMode} > - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => ( = ({ )}
- {dropIndicatorProps &&
} )} - + ); }; diff --git a/superset-frontend/src/dashboard/components/gridComponents/Column.jsx b/superset-frontend/src/dashboard/components/gridComponents/Column.jsx index 1883531404f77..3511c54a994fb 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 ( - - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => ( + {editMode && ( + 0 && 'droptarget-edge', + )} + editMode + > + {({ dropIndicatorProps }) => + dropIndicatorProps &&
+ } + + )} {columnItems.length === 0 ? (
{t('Empty column')}
) : ( columnItems.map((componentId, itemIndex) => ( - + + + {editMode && ( + + {({ dropIndicatorProps }) => + dropIndicatorProps && ( +
+ ) + } + + )} + )) )} - - {dropIndicatorProps &&
} )} - + ); } } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Column.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Column.test.jsx index 2d759b812a432..b1521211ada07 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 }) => ( +
{children({})}
+ ), + Droppable: ({ children }) => ( +
{children({})}
+ ), +})); 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 078405be3e4a3..638527aa3b856 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 ( - - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => (
{editMode && ( )} - - - {dropIndicatorProps &&
}
)} - + ); } } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Divider.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Divider.test.jsx index 6331f68832950..f98378ab72954 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 707597d8c0caa..f1752588d2da9 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 = ({ ); return ( - = ({ onDrop={handleComponentDrop} editMode={editMode} > - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => ( = ({
- {dropIndicatorProps &&
} )} - + ); }; export default DynamicComponent; diff --git a/superset-frontend/src/dashboard/components/gridComponents/Header.jsx b/superset-frontend/src/dashboard/components/gridComponents/Header.jsx index 253f377fc2f7d..85f4cd7cc7304 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 ( - - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => (
{editMode && depth <= 2 && ( // drag handle looks bad when nested @@ -239,11 +239,9 @@ class Header extends React.PureComponent { )} - - {dropIndicatorProps &&
}
)} - + ); } } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Header.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Header.test.jsx index 6f903a05c1c23..64ff712129ca1 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 9febfacf909dc..23a06a0f7d1ad 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 ( - - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => ( - {dropIndicatorProps &&
} )} - + ); } } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Markdown.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Markdown.test.jsx index ac97c38a4afec..b15c4c504f69f 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( @@ -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 d645a1a2cfc6f..95560e3b5900b 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 ( - - {({ dropIndicatorProps, dragSourceRef }) => ( + {({ dragSourceRef }) => ( - {rowItems.length === 0 ? ( + {editMode && ( + 0 && 'droptarget-side', + )} + editMode + style={{ + height: rowItems.length > 0 ? containerHeight : '100%', + ...(rowItems.length > 0 && { width: 16 }), + }} + > + {({ dropIndicatorProps }) => + dropIndicatorProps &&
+ } + + )} + {rowItems.length === 0 && (
{t('Empty row')}
- ) : ( - rowItems.map((componentId, itemIndex) => ( - - )) )} - - {dropIndicatorProps &&
} + {rowItems.length > 0 && + rowItems.map((componentId, itemIndex) => ( + + + {editMode && ( + + {({ dropIndicatorProps }) => + dropIndicatorProps &&
+ } + + )} + + ))} )} - + ); } } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Row.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Row.test.jsx index b69a8f7e97348..e330195d77555 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 }) => ( +
{children({})}
+ ), + Droppable: ({ children }) => ( +
{children({})}
+ ), +})); 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 d1d08176baa93..b546947e31a4f 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 && ( -
- ); - -const renderDraggableContentTop = dropProps => - dropProps.dropIndicatorProps && ( -
- ); +const renderDraggableContent = dropProps => + 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 {
{/* Make top of tab droppable */} {editMode && ( - - {renderDraggableContentTop} - + {renderDraggableContent} + )} {shouldDisplayEmptyState && ( )} {tabComponent.children.map((componentId, componentIndex) => ( - + + + {/* Make bottom of tab droppable */} + {editMode && ( + + {renderDraggableContent} + + )} + ))} - {/* Make bottom of tab droppable */} - {editMode && tabComponent.children.length > 0 && ( - - {renderDraggableContentBottom} - - )}
); } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tab.test.tsx b/superset-frontend/src/dashboard/components/gridComponents/Tab.test.tsx index d995595c49100..e19a086d811ce 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', () => )), ); -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 (
- {props.children(childProps)}
); }), -); +})); 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(, { useRedux: true, useDnd: true }); + const { getByTestId } = render(, { + 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(, { useRedux: true, useDnd: true }); + const { getByTestId } = render(, { + 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(, { useRedux: true, useDnd: true }); + const { getByTestId } = render(, { + 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(, { useRedux: true, useDnd: true }); + const { queryByTestId } = render(, { + 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(, { useRedux: true, useDnd: true }); + const { getAllByTestId } = render(, { + 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(, { useRedux: true, useDnd: true }); + const { getAllByTestId, rerender } = render( + , + { + 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(); + 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 67f4b3c598bd9..c9f35d3ba56fc 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 ( - - {({ - dropIndicatorProps: tabsDropIndicatorProps, - dragSourceRef: tabsDragSourceRef, - }) => ( + {({ dragSourceRef: tabsDragSourceRef }) => ( ))} - - {/* don't indicate that a drop on root is allowed when tabs already exist */} - {tabsDropIndicatorProps && - parentComponent.id !== DASHBOARD_ROOT_ID && ( -
- )} )} - + ); } } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tabs.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Tabs.test.jsx index 127b4d42db646..7f39bdcc68079 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 }) => ( +
{children({})}
+ ), + Droppable: ({ children }) => ( +
{children({})}
+ ), +})); jest.mock('src/dashboard/containers/DashboardComponent', () => ({ id }) => (
{id}
)); @@ -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 9ee9fc6866dbc..6aef193b44962 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', () =>
); }), -); +})); const createProps = () => ({ id: 'TABS-L-d9eyOE-b', @@ -123,7 +123,7 @@ test('Should render editMode:true', () => { const props = createProps(); render(, { 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(, { 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 d4e5fed4b6327..d512c2a8a6cc3 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 81cbc48f46941..c4a21ad113864 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 71f506b1bd7ee..9fdd02afa6f60 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); }); });