feat(dashboard): update tab drag and drop reordering with positional placement and indicators for UI (#29395)

This commit is contained in:
Ross Mabbett 2024-09-30 05:15:39 -04:00 committed by GitHub
parent 999dca76c1
commit bdd50c7553
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 396 additions and 51 deletions

View File

@ -19,6 +19,7 @@
import { getEmptyImage } from 'react-dnd-html5-backend';
import { PureComponent } from 'react';
import PropTypes from 'prop-types';
import { TAB_TYPE } from 'src/dashboard/util/componentTypes';
import { DragSource, DropTarget } from 'react-dnd';
import cx from 'classnames';
import { css, styled } from '@superset-ui/core';
@ -40,6 +41,8 @@ const propTypes = {
style: PropTypes.object,
onDrop: PropTypes.func,
onHover: PropTypes.func,
onDropIndicatorChange: PropTypes.func,
onDragTab: PropTypes.func,
editMode: PropTypes.bool,
useEmptyDragPreview: PropTypes.bool,
@ -47,6 +50,8 @@ const propTypes = {
isDragging: PropTypes.bool,
isDraggingOver: PropTypes.bool,
isDraggingOverShallow: PropTypes.bool,
dragComponentType: PropTypes.string,
dragComponentId: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
droppableRef: PropTypes.func,
dragSourceRef: PropTypes.func,
dragPreviewRef: PropTypes.func,
@ -61,6 +66,8 @@ const defaultProps = {
children() {},
onDrop() {},
onHover() {},
onDropIndicatorChange() {},
onDragTab() {},
orientation: 'row',
useEmptyDragPreview: false,
isDragging: false,
@ -129,6 +136,38 @@ export class UnwrappedDragDroppable extends PureComponent {
this.mounted = false;
}
componentDidUpdate(prevProps, prevState) {
const {
onDropIndicatorChange,
isDraggingOver,
component,
index,
dragComponentId,
onDragTab,
} = this.props;
const { dropIndicator } = this.state;
const isTabsType = component.type === TAB_TYPE;
const validStateChange =
dropIndicator !== prevState.dropIndicator ||
isDraggingOver !== prevProps.isDraggingOver ||
index !== prevProps.index;
if (onDropIndicatorChange && isTabsType && validStateChange) {
onDropIndicatorChange({ dropIndicator, isDraggingOver, index });
}
if (dragComponentId !== prevProps.dragComponentId) {
setTimeout(() => {
/**
* This timeout ensures the dargSourceRef and dragPreviewRef are set
* before the component is removed in Tabs.jsx. Otherwise react-dnd
* will not render the drag preview.
*/
onDragTab(dragComponentId);
});
}
}
setRef(ref) {
this.ref = ref;
// this is needed for a custom drag preview
@ -155,6 +194,8 @@ export class UnwrappedDragDroppable extends PureComponent {
isDraggingOver,
style,
editMode,
component,
dragComponentType,
} = this.props;
const { dropIndicator } = this.state;
@ -168,10 +209,14 @@ export class UnwrappedDragDroppable extends PureComponent {
}
: null;
const draggingTabOnTab =
component.type === TAB_TYPE && dragComponentType === TAB_TYPE;
const childProps = editMode
? {
dragSourceRef,
dropIndicatorProps,
draggingTabOnTab,
}
: {};

View File

@ -23,7 +23,11 @@ import {
import sinon from 'sinon';
import newComponentFactory from 'src/dashboard/util/newComponentFactory';
import { CHART_TYPE, ROW_TYPE } from 'src/dashboard/util/componentTypes';
import {
CHART_TYPE,
ROW_TYPE,
TAB_TYPE,
} from 'src/dashboard/util/componentTypes';
import { UnwrappedDragDroppable as DragDroppable } from 'src/dashboard/components/dnd/DragDroppable';
describe('DragDroppable', () => {
@ -63,6 +67,16 @@ describe('DragDroppable', () => {
expect(childrenSpy.callCount).toBe(1);
});
it('should call onDropIndicatorChange when isDraggingOver changes', () => {
const onDropIndicatorChange = sinon.spy();
const wrapper = setup({
onDropIndicatorChange,
component: newComponentFactory(TAB_TYPE),
});
wrapper.setProps({ isDraggingOver: true });
expect(onDropIndicatorChange.callCount).toBe(1);
});
it('should call its child function with "dragSourceRef" if editMode=true', () => {
const children = sinon.spy();
const dragSourceRef = () => {};

View File

@ -47,6 +47,8 @@ export const dragConfig = [
dragSourceRef: connect.dragSource(),
dragPreviewRef: connect.dragPreview(),
isDragging: monitor.isDragging(),
dragComponentType: monitor.getItem()?.type,
dragComponentId: monitor.getItem()?.id,
};
},
];

View File

@ -53,6 +53,7 @@ export default function handleDrop(props, monitor, Component) {
type: draggingItem.type,
meta: draggingItem.meta,
},
position: dropPosition,
};
const shouldAppendToChildren =

View File

@ -47,6 +47,8 @@ const propTypes = {
depth: PropTypes.number.isRequired,
renderType: PropTypes.oneOf([RENDER_TAB, RENDER_TAB_CONTENT]).isRequired,
onDropOnTab: PropTypes.func,
onDropPositionChange: PropTypes.func,
onDragTab: PropTypes.func,
onHoverTab: PropTypes.func,
editMode: PropTypes.bool.isRequired,
canEdit: PropTypes.bool.isRequired,
@ -69,6 +71,8 @@ const defaultProps = {
availableColumnCount: 0,
columnWidth: 0,
onDropOnTab() {},
onDropPositionChange() {},
onDragTab() {},
onHoverTab() {},
onResizeStart() {},
onResize() {},
@ -86,6 +90,14 @@ const TabTitleContainer = styled.div`
`}
`;
const TitleDropIndicator = styled.div`
&.drop-indicator {
position: absolute;
top: 0;
border-radius: 4px;
}
`;
const renderDraggableContent = dropProps =>
dropProps.dropIndicatorProps && <div {...dropProps.dropIndicatorProps} />;
@ -268,6 +280,8 @@ class Tab extends PureComponent {
editMode,
isFocused,
isHighlighted,
onDropPositionChange,
onDragTab,
} = this.props;
return (
@ -279,10 +293,12 @@ class Tab extends PureComponent {
depth={depth}
onDrop={this.handleDrop}
onHover={this.handleOnHover}
onDropIndicatorChange={onDropPositionChange}
onDragTab={onDragTab}
editMode={editMode}
dropToChild={this.shouldDropToChild}
>
{({ dropIndicatorProps, dragSourceRef }) => (
{({ dropIndicatorProps, dragSourceRef, draggingTabOnTab }) => (
<TabTitleContainer
isHighlighted={isHighlighted}
className="dragdroppable-tab"
@ -304,8 +320,12 @@ class Tab extends PureComponent {
placement={index >= 5 ? 'left' : 'right'}
/>
)}
{dropIndicatorProps && <div {...dropIndicatorProps} />}
{dropIndicatorProps && !draggingTabOnTab && (
<TitleDropIndicator
className={dropIndicatorProps.className}
data-test="title-drop-indicator"
/>
)}
</TabTitleContainer>
)}
</DragDroppable>

View File

@ -196,6 +196,10 @@ test('Drop on a tab', async () => {
);
fireEvent.dragStart(screen.getByText('Dashboard Component'));
fireEvent.dragOver(screen.getByText('Next Tab'));
await waitFor(() =>
expect(screen.getByTestId('title-drop-indicator')).toBeVisible(),
);
fireEvent.drop(screen.getByText('Next Tab'));
await waitFor(() => expect(mockOnDropOnTab).toHaveBeenCalledTimes(2));
expect(mockOnDropOnTab).toHaveBeenLastCalledWith(

View File

@ -21,8 +21,10 @@ import PropTypes from 'prop-types';
import { styled, t } from '@superset-ui/core';
import { connect } from 'react-redux';
import { LineEditableTabs } from 'src/components/Tabs';
import Icons from 'src/components/Icons';
import { LOG_ACTIONS_SELECT_DASHBOARD_TAB } from 'src/logger/LogUtils';
import { AntdModal } from 'src/components';
import { DROP_LEFT, DROP_RIGHT } from 'src/dashboard/util/getDropPosition';
import { Draggable } from '../dnd/DragDroppable';
import DragHandle from '../dnd/DragHandle';
import DashboardComponent from '../../containers/DashboardComponent';
@ -108,6 +110,29 @@ const StyledTabsContainer = styled.div`
}
`;
const StyledCancelXIcon = styled(Icons.CancelX)`
color: ${({ theme }) => theme.colors.grayscale.base};
`;
const DropIndicator = styled.div`
border: 2px solid ${({ theme }) => theme.colors.primary.base};
width: 5px;
height: 100%;
position: absolute;
top: 0;
${({ pos }) => (pos === 'left' ? 'left: -4px' : 'right: -4px')};
border-radius: 2px;
`;
const CloseIconWithDropIndicator = props => (
<>
<StyledCancelXIcon />
{props.showDropIndicators.right && (
<DropIndicator className="drop-indicator-right" pos="right" />
)}
</>
);
export class Tabs extends PureComponent {
constructor(props) {
super(props);
@ -122,6 +147,8 @@ export class Tabs extends PureComponent {
this.handleDeleteTab = this.handleDeleteTab.bind(this);
this.handleDropOnTab = this.handleDropOnTab.bind(this);
this.handleDrop = this.handleDrop.bind(this);
this.handleGetDropPosition = this.handleGetDropPosition.bind(this);
this.handleDragggingTab = this.handleDragggingTab.bind(this);
}
componentDidMount() {
@ -286,6 +313,19 @@ export class Tabs extends PureComponent {
}
}
handleGetDropPosition(dragObject) {
const { dropIndicator, isDraggingOver, index } = dragObject;
if (isDraggingOver) {
this.setState(() => ({
dropPosition: dropIndicator,
dragOverTabIndex: index,
}));
} else {
this.setState(() => ({ dropPosition: null }));
}
}
handleDropOnTab(dropResult) {
const { component } = this.props;
@ -311,6 +351,14 @@ export class Tabs extends PureComponent {
}
}
handleDragggingTab(tabId) {
if (tabId) {
this.setState(() => ({ draggingTabId: tabId }));
} else {
this.setState(() => ({ draggingTabId: null }));
}
}
render() {
const {
depth,
@ -330,7 +378,20 @@ export class Tabs extends PureComponent {
} = this.props;
const { children: tabIds } = tabsComponent;
const { tabIndex: selectedTabIndex, activeKey } = this.state;
const {
tabIndex: selectedTabIndex,
activeKey,
dropPosition,
dragOverTabIndex,
} = this.state;
const showDropIndicators = currentDropTabIndex =>
currentDropTabIndex === dragOverTabIndex && {
left: editMode && dropPosition === DROP_LEFT,
right: editMode && dropPosition === DROP_RIGHT,
};
const removeDraggedTab = tabID => this.state.draggingTabId === tabID;
let tabsToHighlight;
const highlightedFilterId =
@ -374,21 +435,47 @@ export class Tabs extends PureComponent {
<LineEditableTabs.TabPane
key={tabId}
tab={
<DashboardComponent
id={tabId}
parentId={tabsComponent.id}
depth={depth}
index={tabIndex}
renderType={RENDER_TAB}
availableColumnCount={availableColumnCount}
columnWidth={columnWidth}
onDropOnTab={this.handleDropOnTab}
onHoverTab={() => this.handleClickTab(tabIndex)}
isFocused={activeKey === tabId}
isHighlighted={
activeKey !== tabId && tabsToHighlight?.includes(tabId)
}
/>
removeDraggedTab(tabId) ? (
<></>
) : (
<>
{showDropIndicators(tabIndex).left && (
<DropIndicator
className="drop-indicator-left"
pos="left"
/>
)}
<DashboardComponent
id={tabId}
parentId={tabsComponent.id}
depth={depth}
index={tabIndex}
renderType={RENDER_TAB}
availableColumnCount={availableColumnCount}
columnWidth={columnWidth}
onDropOnTab={this.handleDropOnTab}
onDropPositionChange={this.handleGetDropPosition}
onDragTab={this.handleDragggingTab}
onHoverTab={() => this.handleClickTab(tabIndex)}
isFocused={activeKey === tabId}
isHighlighted={
activeKey !== tabId &&
tabsToHighlight?.includes(tabId)
}
/>
</>
)
}
closeIcon={
removeDraggedTab(tabId) ? (
<></>
) : (
<CloseIconWithDropIndicator
role="button"
tabIndex={tabIndex}
showDropIndicators={showDropIndicators(tabIndex)}
/>
)
}
>
{renderTabContent && (

View File

@ -122,21 +122,24 @@ test('Should render editMode:true', () => {
const props = createProps();
render(<Tabs {...props} />, { useRedux: true, useDnd: true });
expect(screen.getAllByRole('tab')).toHaveLength(3);
expect(Draggable).toBeCalledTimes(1);
expect(DashboardComponent).toBeCalledTimes(4);
expect(DeleteComponentButton).toBeCalledTimes(1);
expect(screen.getAllByRole('button', { name: 'remove' })).toHaveLength(3);
expect(screen.getAllByRole('button', { name: 'Add tab' })).toHaveLength(2);
expect(Draggable).toHaveBeenCalledTimes(1);
expect(DashboardComponent).toHaveBeenCalledTimes(4);
expect(DeleteComponentButton).toHaveBeenCalledTimes(1);
});
test('Should render editMode:false', () => {
const props = createProps();
props.editMode = false;
render(<Tabs {...props} />, { useRedux: true, useDnd: true });
render(<Tabs {...props} />, {
useRedux: true,
useDnd: true,
});
expect(screen.getAllByRole('tab')).toHaveLength(3);
expect(Draggable).toBeCalledTimes(1);
expect(DashboardComponent).toBeCalledTimes(4);
expect(DeleteComponentButton).not.toBeCalled();
expect(Draggable).toHaveBeenCalledTimes(1);
expect(DashboardComponent).toHaveBeenCalledTimes(4);
expect(DeleteComponentButton).not.toHaveBeenCalled();
expect(
screen.queryByRole('button', { name: 'remove' }),
).not.toBeInTheDocument();
@ -153,11 +156,11 @@ test('Update component props', () => {
useRedux: true,
useDnd: true,
});
expect(DeleteComponentButton).not.toBeCalled();
expect(DeleteComponentButton).not.toHaveBeenCalled();
props.editMode = true;
rerender(<Tabs {...props} />);
expect(DeleteComponentButton).toBeCalledTimes(1);
expect(DeleteComponentButton).toHaveBeenCalledTimes(1);
});
test('Clicking on "DeleteComponentButton"', () => {
@ -167,9 +170,12 @@ test('Clicking on "DeleteComponentButton"', () => {
useDnd: true,
});
expect(props.deleteComponent).not.toBeCalled();
expect(props.deleteComponent).not.toHaveBeenCalled();
userEvent.click(screen.getByTestId('DeleteComponentButton'));
expect(props.deleteComponent).toBeCalledWith('TABS-L-d9eyOE-b', 'GRID_ID');
expect(props.deleteComponent).toHaveBeenCalledWith(
'TABS-L-d9eyOE-b',
'GRID_ID',
);
});
test('Add new tab', () => {
@ -179,9 +185,9 @@ test('Add new tab', () => {
useDnd: true,
});
expect(props.createComponent).not.toBeCalled();
expect(props.createComponent).not.toHaveBeenCalled();
userEvent.click(screen.getAllByRole('button', { name: 'Add tab' })[0]);
expect(props.createComponent).toBeCalled();
expect(props.createComponent).toHaveBeenCalled();
});
test('Removing a tab', async () => {
@ -191,16 +197,16 @@ test('Removing a tab', async () => {
useDnd: true,
});
expect(props.deleteComponent).not.toBeCalled();
expect(props.deleteComponent).not.toHaveBeenCalled();
expect(screen.queryByText('Delete dashboard tab?')).not.toBeInTheDocument();
userEvent.click(screen.getAllByRole('button', { name: 'remove' })[0]);
expect(props.deleteComponent).not.toBeCalled();
expect(props.deleteComponent).not.toHaveBeenCalled();
expect(await screen.findByText('Delete dashboard tab?')).toBeInTheDocument();
expect(props.deleteComponent).not.toBeCalled();
expect(props.deleteComponent).not.toHaveBeenCalled();
userEvent.click(screen.getByRole('button', { name: 'DELETE' }));
expect(props.deleteComponent).toBeCalled();
expect(props.deleteComponent).toHaveBeenCalled();
});
test('Switching tabs', () => {
@ -210,11 +216,11 @@ test('Switching tabs', () => {
useDnd: true,
});
expect(props.logEvent).not.toBeCalled();
expect(props.onChangeTab).not.toBeCalled();
expect(props.logEvent).not.toHaveBeenCalled();
expect(props.onChangeTab).not.toHaveBeenCalled();
userEvent.click(screen.getAllByRole('tab')[2]);
expect(props.logEvent).toBeCalled();
expect(props.onChangeTab).toBeCalled();
expect(props.logEvent).toHaveBeenCalled();
expect(props.onChangeTab).toHaveBeenCalled();
});
test('Call "DashboardComponent.onDropOnTab"', async () => {
@ -224,12 +230,12 @@ test('Call "DashboardComponent.onDropOnTab"', async () => {
useDnd: true,
});
expect(props.logEvent).not.toBeCalled();
expect(props.onChangeTab).not.toBeCalled();
expect(props.logEvent).not.toHaveBeenCalled();
expect(props.onChangeTab).not.toHaveBeenCalled();
userEvent.click(screen.getAllByText('DashboardComponent')[0]);
await waitFor(() => {
expect(props.logEvent).toBeCalled();
expect(props.onChangeTab).toBeCalled();
expect(props.logEvent).toHaveBeenCalled();
expect(props.onChangeTab).toHaveBeenCalled();
});
});

View File

@ -134,7 +134,7 @@ const actionHandlers = {
const {
payload: { dropResult },
} = action;
const { source, destination, dragging } = dropResult;
const { source, destination, dragging, position } = dropResult;
if (!source || !destination || !dragging) return state;
@ -142,6 +142,7 @@ const actionHandlers = {
entitiesMap: state,
source,
destination,
position,
});
if (componentIsResizable(nextEntities[dragging.id])) {

View File

@ -16,6 +16,10 @@
* specific language governing permissions and limitations
* under the License.
*/
import { TABS_TYPE } from './componentTypes';
import { DROP_LEFT, DROP_RIGHT } from './getDropPosition';
export function reorder(list, startIndex, endIndex) {
const result = [...list];
const [removed] = result.splice(startIndex, 1);
@ -24,14 +28,49 @@ export function reorder(list, startIndex, endIndex) {
return result;
}
export default function reorderItem({ entitiesMap, source, destination }) {
export default function reorderItem({
entitiesMap,
source,
destination,
position,
}) {
const current = [...entitiesMap[source.id].children];
const next = [...entitiesMap[destination.id].children];
const target = current[source.index];
const isSameSource = source.id === destination.id;
const isTabsType = source.type && destination.type === TABS_TYPE;
// moving to same list
if (source.id === destination.id) {
const reordered = reorder(current, source.index, destination.index);
let dropIndex = destination.index;
if (isSameSource) {
if (isTabsType) {
if (position === DROP_LEFT) {
dropIndex = Math.max(dropIndex, 0);
} else if (position === DROP_RIGHT) {
dropIndex += 1;
}
const isRightPosition =
position === DROP_RIGHT && source.index === destination.index + 1;
const isLeftPosition =
position === DROP_LEFT && source.index === destination.index - 1;
const sameTabSourceIndex = isRightPosition || isLeftPosition;
if (sameTabSourceIndex) {
// If the source tab is dropped to be the same index as the source
// tab, no change is needed in entitiesMap
return entitiesMap;
}
// Adjust dropIndex to account for the source tab being removed
if (dropIndex > source.index) {
dropIndex -= 1;
}
}
const reordered = reorder(current, source.index, dropIndex);
const result = {
...entitiesMap,
@ -44,9 +83,18 @@ export default function reorderItem({ entitiesMap, source, destination }) {
return result;
}
if (isTabsType) {
// Ensure the dropIndex is within the bounds of the destination children
if (position === DROP_LEFT) {
dropIndex = Math.max(dropIndex, 0);
} else if (position === DROP_RIGHT) {
dropIndex = Math.min(dropIndex + 1, current.length - 1);
}
}
// moving to different list
current.splice(source.index, 1); // remove from original
next.splice(destination.index, 0, target); // insert into next
next.splice(dropIndex, 0, target); // insert into next
const result = {
...entitiesMap,

View File

@ -17,6 +17,8 @@
* under the License.
*/
import reorderItem from 'src/dashboard/util/dnd-reorder';
import { TABS_TYPE } from './componentTypes';
import { DROP_LEFT, DROP_RIGHT } from './getDropPosition';
describe('dnd-reorderItem', () => {
it('should remove the item from its source entity and add it to its destination entity', () => {
@ -72,6 +74,121 @@ describe('dnd-reorderItem', () => {
destination: { id: 'b', index: 1 },
});
expect(result.iAmExtra === extraEntity).toBe(true);
expect(result.iAmExtra).toBe(extraEntity);
});
it('should handle out of bounds destination index gracefully', () => {
const result = reorderItem({
entitiesMap: {
a: {
id: 'a',
children: ['x', 'y', 'z'],
},
b: {
id: 'b',
children: ['banana'],
},
},
source: { id: 'a', index: 1 },
destination: { id: 'b', index: 5 },
});
expect(result.a.children).toEqual(['x', 'z']);
expect(result.b.children).toEqual(['banana', 'y']);
});
it('should do nothing if source and destination are the same and indices are the same', () => {
const result = reorderItem({
entitiesMap: {
a: {
id: 'a',
children: ['x', 'y', 'z'],
},
},
source: { id: 'a', index: 1 },
destination: { id: 'a', index: 1 },
});
expect(result.a.children).toEqual(['x', 'y', 'z']);
});
it('should handle DROP_LEFT in the same TABS_TYPE list correctly', () => {
const result = reorderItem({
entitiesMap: {
a: {
id: 'a',
type: TABS_TYPE,
children: ['x', 'y', 'z'],
},
},
source: { id: 'a', type: TABS_TYPE, index: 2 },
destination: { id: 'a', type: TABS_TYPE, index: 1 },
position: DROP_LEFT,
});
expect(result.a.children).toEqual(['x', 'z', 'y']);
});
it('should handle DROP_RIGHT in the same TABS_TYPE list correctly', () => {
const result = reorderItem({
entitiesMap: {
a: {
id: 'a',
type: TABS_TYPE,
children: ['x', 'y', 'z'],
},
},
source: { id: 'a', type: TABS_TYPE, index: 0 },
destination: { id: 'a', type: TABS_TYPE, index: 1 },
position: DROP_RIGHT,
});
expect(result.a.children).toEqual(['y', 'x', 'z']);
});
it('should handle DROP_LEFT when moving between different TABS_TYPE lists', () => {
const result = reorderItem({
entitiesMap: {
a: {
id: 'a',
type: TABS_TYPE,
children: ['x', 'y'],
},
b: {
id: 'b',
type: TABS_TYPE,
children: ['banana'],
},
},
source: { id: 'a', type: TABS_TYPE, index: 1 },
destination: { id: 'b', type: TABS_TYPE, index: 0 },
position: DROP_LEFT,
});
expect(result.a.children).toEqual(['x']);
expect(result.b.children).toEqual(['y', 'banana']);
});
it('should handle DROP_RIGHT when moving between different TABS_TYPE lists', () => {
const result = reorderItem({
entitiesMap: {
a: {
id: 'a',
type: TABS_TYPE,
children: ['x', 'y'],
},
b: {
id: 'b',
type: TABS_TYPE,
children: ['banana'],
},
},
source: { id: 'a', type: TABS_TYPE, index: 0 },
destination: { id: 'b', type: TABS_TYPE, index: 0 },
position: DROP_RIGHT,
});
expect(result.a.children).toEqual(['y']);
expect(result.b.children).toEqual(['banana', 'x']);
});
});