superset-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ccwilli...@apache.org
Subject [incubator-superset] 02/02: [dnd] add depth validation to drag and drop logic
Date Sat, 17 Mar 2018 00:08:35 GMT
This is an automated email from the ASF dual-hosted git repository.

ccwilliams pushed a commit to branch chris--top-level-tabs
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git

commit bcac78cfcfb271c4876136207654e9d19e4ebdd3
Author: Chris Williams <chris.williams@airbnb.com>
AuthorDate: Fri Mar 16 17:08:14 2018 -0700

    [dnd] add depth validation to drag and drop logic
---
 .../dashboard/v2/components/DashboardBuilder.jsx   |  7 +-
 .../dashboard/v2/components/DashboardGrid.jsx      | 11 ++-
 .../dashboard/v2/components/dnd/DragDroppable.jsx  |  1 +
 .../v2/components/gridComponents/Chart.jsx         |  5 +-
 .../v2/components/gridComponents/Column.jsx        |  1 +
 .../v2/components/gridComponents/Divider.jsx       |  3 +
 .../v2/components/gridComponents/Header.jsx        |  3 +
 .../dashboard/v2/components/gridComponents/Row.jsx |  1 +
 .../v2/components/gridComponents/Spacer.jsx        |  1 +
 .../dashboard/v2/components/gridComponents/Tab.jsx |  4 +-
 .../v2/components/gridComponents/Tabs.jsx          |  3 +-
 .../gridComponents/new/DraggableNewComponent.jsx   |  1 +
 .../dashboard/v2/stylesheets/hover-menu.less       |  4 +-
 .../javascripts/dashboard/v2/util/constants.js     |  1 +
 .../dashboard/v2/util/getDropPosition.js           | 12 ++-
 .../javascripts/dashboard/v2/util/isValidChild.js  | 89 +++++++++++++---------
 16 files changed, 100 insertions(+), 47 deletions(-)

diff --git a/superset/assets/javascripts/dashboard/v2/components/DashboardBuilder.jsx b/superset/assets/javascripts/dashboard/v2/components/DashboardBuilder.jsx
index 51e1897..e011ad4 100644
--- a/superset/assets/javascripts/dashboard/v2/components/DashboardBuilder.jsx
+++ b/superset/assets/javascripts/dashboard/v2/components/DashboardBuilder.jsx
@@ -14,6 +14,7 @@ import WithPopoverMenu from './menu/WithPopoverMenu';
 import {
   DASHBOARD_GRID_ID,
   DASHBOARD_ROOT_ID,
+  DASHBOARD_ROOT_DEPTH,
 } from '../util/constants';
 
 const propTypes = {
@@ -65,12 +66,13 @@ class DashboardBuilder extends React.Component {
 
     return (
       <div className="dashboard-v2">
-        {topLevelTabs ? ( // you cannot displace tabs if they already exist
+        {topLevelTabs ? ( // you cannot drop on/displace tabs if they already exist
           <DashboardHeader />
         ) : (
           <DragDroppable
             component={dashboardRoot}
             parentComponent={null}
+            depth={DASHBOARD_ROOT_DEPTH}
             index={0}
             orientation="column"
             onDrop={topLevelTabs ? null : handleComponentDrop}
@@ -97,7 +99,7 @@ class DashboardBuilder extends React.Component {
             <DashboardComponent
               id={topLevelTabs.id}
               parentId={DASHBOARD_ROOT_ID}
-              depth={0}
+              depth={DASHBOARD_ROOT_DEPTH + 1}
               index={0}
               renderTabContent={false}
               onChangeTab={this.handleChangeTab}
@@ -110,6 +112,7 @@ class DashboardBuilder extends React.Component {
             dashboard={dashboard}
             handleComponentDrop={handleComponentDrop}
             updateComponents={updateComponents}
+            depth={DASHBOARD_ROOT_DEPTH + 1}
           />
           <BuilderComponentPane />
         </div>
diff --git a/superset/assets/javascripts/dashboard/v2/components/DashboardGrid.jsx b/superset/assets/javascripts/dashboard/v2/components/DashboardGrid.jsx
index dfba72a..7aad027 100644
--- a/superset/assets/javascripts/dashboard/v2/components/DashboardGrid.jsx
+++ b/superset/assets/javascripts/dashboard/v2/components/DashboardGrid.jsx
@@ -2,19 +2,21 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import ParentSize from '@vx/responsive/build/components/ParentSize';
 
+import { componentShape } from '../util/propShapes';
 import DashboardComponent from '../containers/DashboardComponent';
 import DragDroppable from './dnd/DragDroppable';
 
 import {
-  // DASHBOARD_ROOT_ID,
   GRID_GUTTER_SIZE,
   GRID_COLUMN_COUNT,
 } from '../util/constants';
 
 const propTypes = {
   dashboard: PropTypes.object.isRequired,
-  updateComponents: PropTypes.func.isRequired,
+  depth: PropTypes.number.isRequired,
+  gridComponent: componentShape.isRequired,
   handleComponentDrop: PropTypes.func.isRequired,
+  updateComponents: PropTypes.func.isRequired,
 };
 
 const defaultProps = {
@@ -84,7 +86,7 @@ class DashboardGrid extends React.PureComponent {
   }
 
   render() {
-    const { gridComponent, handleComponentDrop } = this.props;
+    const { gridComponent, handleComponentDrop, depth } = this.props;
     const { isResizing, rowGuideTop } = this.state;
 
     return (
@@ -102,7 +104,7 @@ class DashboardGrid extends React.PureComponent {
                     key={id}
                     id={id}
                     parentId={gridComponent.id}
-                    depth={0}
+                    depth={depth + 1}
                     index={index}
                     availableColumnCount={GRID_COLUMN_COUNT}
                     columnWidth={columnWidth}
@@ -116,6 +118,7 @@ class DashboardGrid extends React.PureComponent {
                 {gridComponent.children.length === 0 &&
                   <DragDroppable
                     component={gridComponent}
+                    depth={depth}
                     parentComponent={null}
                     index={0}
                     orientation="column"
diff --git a/superset/assets/javascripts/dashboard/v2/components/dnd/DragDroppable.jsx b/superset/assets/javascripts/dashboard/v2/components/dnd/DragDroppable.jsx
index 511d67b..89664e5 100644
--- a/superset/assets/javascripts/dashboard/v2/components/dnd/DragDroppable.jsx
+++ b/superset/assets/javascripts/dashboard/v2/components/dnd/DragDroppable.jsx
@@ -12,6 +12,7 @@ const propTypes = {
   className: PropTypes.string,
   component: componentShape.isRequired,
   parentComponent: componentShape,
+  depth: PropTypes.number.isRequired,
   disableDragDrop: PropTypes.bool,
   orientation: PropTypes.oneOf(['row', 'column']),
   index: PropTypes.number.isRequired,
diff --git a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Chart.jsx
b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Chart.jsx
index 9daa8cf..7ca506d 100644
--- a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Chart.jsx
+++ b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Chart.jsx
@@ -8,7 +8,7 @@ import HoverMenu from '../menu/HoverMenu';
 import ResizableContainer from '../resizable/ResizableContainer';
 import WithPopoverMenu from '../menu/WithPopoverMenu';
 import { componentShape } from '../../util/propShapes';
-
+import { ROW_TYPE } from '../../util/componentTypes';
 import {
   GRID_MIN_COLUMN_COUNT,
   GRID_MIN_ROW_UNITS,
@@ -79,13 +79,14 @@ class Chart extends React.Component {
         parentComponent={parentComponent}
         orientation={depth % 2 === 1 ? 'column' : 'row'}
         index={index}
+        depth={depth}
         onDrop={handleComponentDrop}
         disableDragDrop={isFocused}
       >
         {({ dropIndicatorProps, dragSourceRef }) => (
           <ResizableContainer
             id={component.id}
-            adjustableWidth={depth <= 1}
+            adjustableWidth={parentComponent.type === ROW_TYPE}
             adjustableHeight
             widthStep={columnWidth}
             widthMultiple={component.meta.width}
diff --git a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Column.jsx
b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Column.jsx
index 8409bc1..2ac810c 100644
--- a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Column.jsx
+++ b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Column.jsx
@@ -80,6 +80,7 @@ class Column extends React.PureComponent {
         parentComponent={parentComponent}
         orientation="column"
         index={index}
+        depth={depth}
         onDrop={handleComponentDrop}
       >
         {({ dropIndicatorProps, dragSourceRef }) => (
diff --git a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Divider.jsx
b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Divider.jsx
index 29437e1..ff29c3f 100644
--- a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Divider.jsx
+++ b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Divider.jsx
@@ -10,6 +10,7 @@ const propTypes = {
   id: PropTypes.string.isRequired,
   parentId: PropTypes.string.isRequired,
   component: componentShape.isRequired,
+  depth: PropTypes.number.isRequired,
   parentComponent: componentShape.isRequired,
   index: PropTypes.number.isRequired,
   handleComponentDrop: PropTypes.func.isRequired,
@@ -30,6 +31,7 @@ class Divider extends React.PureComponent {
   render() {
     const {
       component,
+      depth,
       parentComponent,
       index,
       handleComponentDrop,
@@ -41,6 +43,7 @@ class Divider extends React.PureComponent {
         parentComponent={parentComponent}
         orientation="row"
         index={index}
+        depth={depth}
         onDrop={handleComponentDrop}
       >
         {({ dropIndicatorProps, dragSourceRef }) => (
diff --git a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Header.jsx
b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Header.jsx
index 967b483..24d2270 100644
--- a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Header.jsx
+++ b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Header.jsx
@@ -19,6 +19,7 @@ const propTypes = {
   id: PropTypes.string.isRequired,
   parentId: PropTypes.string.isRequired,
   component: componentShape.isRequired,
+  depth: PropTypes.number.isRequired,
   parentComponent: componentShape.isRequired,
   index: PropTypes.number.isRequired,
 
@@ -74,6 +75,7 @@ class Header extends React.PureComponent {
 
     const {
       component,
+      depth,
       parentComponent,
       index,
       handleComponentDrop,
@@ -93,6 +95,7 @@ class Header extends React.PureComponent {
         parentComponent={parentComponent}
         orientation="row"
         index={index}
+        depth={depth}
         onDrop={handleComponentDrop}
         disableDragDrop={isFocused}
       >
diff --git a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Row.jsx b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Row.jsx
index 3386f8c..e766eae 100644
--- a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Row.jsx
+++ b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Row.jsx
@@ -116,6 +116,7 @@ class Row extends React.PureComponent {
         parentComponent={parentComponent}
         orientation="row"
         index={index}
+        depth={depth}
         onDrop={handleComponentDrop}
       >
         {({ dropIndicatorProps, dragSourceRef }) => (
diff --git a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Spacer.jsx
b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Spacer.jsx
index faac589..7693f9c 100644
--- a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Spacer.jsx
+++ b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Spacer.jsx
@@ -69,6 +69,7 @@ class Spacer extends React.PureComponent {
         parentComponent={parentComponent}
         orientation={orientation}
         index={index}
+        depth={depth}
         onDrop={handleComponentDrop}
       >
         {({ dropIndicatorProps, dragSourceRef }) => (
diff --git a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Tab.jsx b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Tab.jsx
index 25f8a9b..9548a4b 100644
--- a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Tab.jsx
+++ b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Tab.jsx
@@ -105,7 +105,7 @@ export default class Tab extends React.PureComponent {
             key={componentId}
             id={componentId}
             parentId={tabComponent.id}
-            depth={depth}
+            depth={depth} // see isValidChild.js for why tabs don't increment child depth
             index={componentIndex}
             onDrop={this.handleDrop}
             availableColumnCount={availableColumnCount}
@@ -125,6 +125,7 @@ export default class Tab extends React.PureComponent {
       component,
       parentComponent,
       index,
+      depth,
     } = this.props;
 
     return (
@@ -133,6 +134,7 @@ export default class Tab extends React.PureComponent {
         parentComponent={parentComponent}
         orientation="column"
         index={index}
+        depth={depth}
         onDrop={this.handleDrop}
         disableDragDrop={isFocused}
       >
diff --git a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Tabs.jsx b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Tabs.jsx
index d1c7cf1..cc5f637 100644
--- a/superset/assets/javascripts/dashboard/v2/components/gridComponents/Tabs.jsx
+++ b/superset/assets/javascripts/dashboard/v2/components/gridComponents/Tabs.jsx
@@ -143,6 +143,7 @@ class Tabs extends React.PureComponent {
         parentComponent={parentComponent}
         orientation="row"
         index={index}
+        depth={depth}
         onDrop={handleComponentDrop}
       >
         {({ dropIndicatorProps: tabsDropIndicatorProps, dragSourceRef: tabsDragSourceRef
}) => (
@@ -188,7 +189,7 @@ class Tabs extends React.PureComponent {
                     <DashboardComponent
                       id={tabId}
                       parentId={tabsComponent.id}
-                      depth={depth}
+                      depth={depth} // see isValidChild.js for why tabs don't increment child
depth
                       index={tabIndex}
                       renderType={RENDER_TAB_CONTENT}
                       availableColumnCount={availableColumnCount}
diff --git a/superset/assets/javascripts/dashboard/v2/components/gridComponents/new/DraggableNewComponent.jsx
b/superset/assets/javascripts/dashboard/v2/components/gridComponents/new/DraggableNewComponent.jsx
index 465209d..778f58e 100644
--- a/superset/assets/javascripts/dashboard/v2/components/gridComponents/new/DraggableNewComponent.jsx
+++ b/superset/assets/javascripts/dashboard/v2/components/gridComponents/new/DraggableNewComponent.jsx
@@ -25,6 +25,7 @@ export default class DraggableNewComponent extends React.PureComponent {
         component={{ type, id }}
         parentComponent={{ id: NEW_COMPONENTS_SOURCE_ID, type: NEW_COMPONENT_SOURCE_TYPE
}}
         index={0}
+        depth={0}
       >
         {({ dragSourceRef }) => (
           <div ref={dragSourceRef} className="new-component">
diff --git a/superset/assets/javascripts/dashboard/v2/stylesheets/hover-menu.less b/superset/assets/javascripts/dashboard/v2/stylesheets/hover-menu.less
index fed5cb7..c308f80 100644
--- a/superset/assets/javascripts/dashboard/v2/stylesheets/hover-menu.less
+++ b/superset/assets/javascripts/dashboard/v2/stylesheets/hover-menu.less
@@ -16,7 +16,7 @@
 }
 
 .hover-menu--left > :nth-child(n):not(:only-child):not(:last-child) {
-  margin-bottom: 8px;
+  margin-bottom: 12px;
 }
 
 .dragdroppable-row .dragdroppable-row .hover-menu--left {
@@ -35,7 +35,7 @@
 }
 
 .hover-menu--top > :nth-child(n):not(:only-child):not(:last-child) {
-  margin-right: 8px;
+  margin-right: 12px;
 }
 
 .dragdroppable:hover .hover-menu,
diff --git a/superset/assets/javascripts/dashboard/v2/util/constants.js b/superset/assets/javascripts/dashboard/v2/util/constants.js
index b57ed93..1108b21 100644
--- a/superset/assets/javascripts/dashboard/v2/util/constants.js
+++ b/superset/assets/javascripts/dashboard/v2/util/constants.js
@@ -15,6 +15,7 @@ export const NEW_TAB_ID = 'NEW_TAB_ID';
 export const NEW_TABS_ID = 'NEW_TABS_ID';
 
 // grid constants
+export const DASHBOARD_ROOT_DEPTH = 0;
 export const GRID_BASE_UNIT = 8;
 export const GRID_GUTTER_SIZE = 2 * GRID_BASE_UNIT;
 export const GRID_ROW_HEIGHT_UNIT = 2 * GRID_BASE_UNIT;
diff --git a/superset/assets/javascripts/dashboard/v2/util/getDropPosition.js b/superset/assets/javascripts/dashboard/v2/util/getDropPosition.js
index 9922d1b..f259cb8 100644
--- a/superset/assets/javascripts/dashboard/v2/util/getDropPosition.js
+++ b/superset/assets/javascripts/dashboard/v2/util/getDropPosition.js
@@ -1,4 +1,5 @@
 import isValidChild from './isValidChild';
+import { TAB_TYPE, TABS_TYPE } from './componentTypes';
 
 export const DROP_TOP = 'DROP_TOP';
 export const DROP_RIGHT = 'DROP_RIGHT';
@@ -9,6 +10,7 @@ const SIBLING_DROP_THRESHOLD = 15;
 
 export default function getDropPosition(monitor, Component) {
   const {
+    depth: componentDepth,
     parentComponent,
     component,
     orientation,
@@ -22,13 +24,21 @@ export default function getDropPosition(monitor, Component) {
     return null;
   }
 
+  debugger;
+
   const validChild = isValidChild({
     parentType: component.type,
+    parentDepth: componentDepth,
     childType: draggingItem.type,
   });
 
+  const parentType = parentComponent && parentComponent.type;
+  const parentDepth = // see isValidChild.js for why tabs don't increment child depth
+    componentDepth + (parentType === TAB_TYPE || parentType === TABS_TYPE ? 0 : -1);
+
   const validSibling = isValidChild({
-    parentType: parentComponent && parentComponent.type,
+    parentType,
+    parentDepth,
     childType: draggingItem.type,
   });
 
diff --git a/superset/assets/javascripts/dashboard/v2/util/isValidChild.js b/superset/assets/javascripts/dashboard/v2/util/isValidChild.js
index c966db0..9c6ae8e 100644
--- a/superset/assets/javascripts/dashboard/v2/util/isValidChild.js
+++ b/superset/assets/javascripts/dashboard/v2/util/isValidChild.js
@@ -1,3 +1,19 @@
+/* eslint max-len: 0 */
+/**
+  * When determining if a component is a valid child of another component we must consider
both
+  *   - parent + child component types
+  *   - component depth, or depth of nesting of container components
+  *
+  * We consider types because some components aren't containers (e.g. a heading) and we consider
+  * depth to prevent infinite nesting of container components.
+  *
+  * The following example container nestings should be valid, which means that some containers
+  * don't increase the (depth) of their children, namely tabs and tab:
+  *   (a) root (0) > grid (1) >                         row (2) > column (3) >
row (4) > non-container (5)
+  *   (b) root (0) > grid (1) >    tabs (2) > tab (2) > row (2) > column (3)
> row (4) > non-container (5)
+  *   (c) root (0) > top-tab (1) >                      row (2) > column (3) >
row (4) > non-container (5)
+  *   (d) root (0) > top-tab (1) > tabs (2) > tab (2) > row (2) > column (3)
> row (4) > non-container (5)
+  */
 import {
   CHART_TYPE,
   COLUMN_TYPE,
@@ -12,49 +28,57 @@ import {
   TAB_TYPE,
 } from './componentTypes';
 
-const typeToValidChildType = {
+import { DASHBOARD_ROOT_DEPTH as rootDepth } from './constants';
+
+const depthOne = rootDepth + 1;
+const depthTwo = rootDepth + 2;
+const depthThree = rootDepth + 3;
+const depthFour = rootDepth + 4;
+
+// when moving components around the depth of child is irrelevant, note these are parent
depths
+const parentMaxDepthLookup = {
   [DASHBOARD_ROOT_TYPE]: {
-    [TABS_TYPE]: true,
-    [DASHBOARD_GRID_TYPE]: true,
+    [TABS_TYPE]: rootDepth,
+    [DASHBOARD_GRID_TYPE]: rootDepth,
   },
 
   [DASHBOARD_GRID_TYPE]: {
-    [CHART_TYPE]: true,
-    [COLUMN_TYPE]: true,
-    [DIVIDER_TYPE]: true,
-    [HEADER_TYPE]: true,
-    [ROW_TYPE]: true,
-    [SPACER_TYPE]: true,
-    [TABS_TYPE]: true,
+    [CHART_TYPE]: depthOne,
+    [COLUMN_TYPE]: depthOne,
+    [DIVIDER_TYPE]: depthOne,
+    [HEADER_TYPE]: depthOne,
+    [ROW_TYPE]: depthOne,
+    [SPACER_TYPE]: depthOne,
+    [TABS_TYPE]: depthOne,
   },
 
   [ROW_TYPE]: {
-    [CHART_TYPE]: true,
-    [MARKDOWN_TYPE]: true,
-    [COLUMN_TYPE]: true,
-    [SPACER_TYPE]: true,
+    [CHART_TYPE]: depthFour,
+    [MARKDOWN_TYPE]: depthFour,
+    [COLUMN_TYPE]: depthTwo,
+    [SPACER_TYPE]: depthFour,
   },
 
   [TABS_TYPE]: {
-    [TAB_TYPE]: true,
+    [TAB_TYPE]: depthTwo,
   },
 
   [TAB_TYPE]: {
-    [CHART_TYPE]: true,
-    [COLUMN_TYPE]: true,
-    [DIVIDER_TYPE]: true,
-    [HEADER_TYPE]: true,
-    [ROW_TYPE]: true,
-    [SPACER_TYPE]: true,
-    [TABS_TYPE]: true,
+    [CHART_TYPE]: depthTwo,
+    [COLUMN_TYPE]: depthTwo,
+    [DIVIDER_TYPE]: depthTwo,
+    [HEADER_TYPE]: depthTwo,
+    [ROW_TYPE]: depthTwo,
+    [SPACER_TYPE]: depthTwo,
+    [TABS_TYPE]: depthTwo,
   },
 
   [COLUMN_TYPE]: {
-    [CHART_TYPE]: true,
-    [HEADER_TYPE]: true,
-    [MARKDOWN_TYPE]: true,
-    [ROW_TYPE]: true,
-    [SPACER_TYPE]: true,
+    [CHART_TYPE]: depthThree,
+    [HEADER_TYPE]: depthThree,
+    [MARKDOWN_TYPE]: depthThree,
+    [ROW_TYPE]: depthThree,
+    [SPACER_TYPE]: depthThree,
   },
 
   // these have no valid children
@@ -65,12 +89,9 @@ const typeToValidChildType = {
   [SPACER_TYPE]: {},
 };
 
-export default function isValidChild({ parentType, childType }) {
-  if (!parentType || !childType) return false;
-
-  const isValid = Boolean(
-    typeToValidChildType[parentType][childType],
-  );
+export default function isValidChild({ parentType, childType, parentDepth }) {
+  if (!parentType || !childType || typeof parentDepth !== 'number') return false;
+  const maxParentDepth = (parentMaxDepthLookup[parentType] || {})[childType];
 
-  return isValid;
+  return typeof maxParentDepth === 'number' && parentDepth <= maxParentDepth;
 }

-- 
To stop receiving notification emails like this one, please contact
ccwilliams@apache.org.

Mime
View raw message