Dashboard builder: fixed 1st round comments #4839

graceguo-supercat · 2018-04-18T05:26:23Z

Todos (after code merge):

move dashboardContainer, sliceAdderContainer to /containers directory
use toast message in dashboard
use payload wrap properties in dashboard actions and reducers

…ader, split pane, Draggable side panel [grid] add <DashboardGrid />, <ResizableContainer />, and initial grid components. [grid] gridComponents/ directory, add fixtures/ directory and test layout, add <Column /> [grid] working grid with gutters [grid] design tweaks and polish, add <Tabs /> [header] add gradient header logo and favicon [dnd] begin adding dnd functionality [dnd] add util/isValidChild.js [react-beautiful-dnd] iterate on dnd until blocked [dnd] refactor to use react-dnd [react-dnd] refactor to use composable <DashboardComponent /> structure [dnd] factor out DashboardComponent, let components render dropInidcator and set draggableRef, add draggable tabs [dnd] refactor to use redux, add DashboardComponent and DashboardGrid containers [dragdroppable] rename horizontal/vertical => row/column [builder] refactor into HoverMenu, add WithPopoverMenu [builder] add editable header and disableDragDrop prop for Dragdroppable's [builder] make tabs editable [builder] add generic popover dropdown and header row style editability [builder] add hover rowStyle dropdown, make row styles editable [builder] add some new component icons, add popover with delete to charts [builder] add preview icons, add popover menu to rows. [builder] add IconButton and RowStyleDropdown [resizable] use ResizableContainer instead of DimensionProvider, fix resize and delete bugs [builder] fix bug with spacer [builder] clean up, header.size => header.headerSize [builder] support more drag/drop combinations by wrapping some components in rows upon drop. fix within list drop index. refactor some utils. [builder][tabs] fix broken add tab button [dashboard builder] don't pass dashboard layout to all dashboard components, improve drop indicator logic, fix delete component pure component bug [dnd] refactor drop position logic

* [top-level-tabs] initial working version of top-level tabs * [top-level-tabs] simplify redux and disable ability to displace top-level tabs with other tabs * [top-level-tabs] improve tab drag and drop css * [undo-redo] add redux undo redo * [dnd] clean up dropResult shape, add new component source id + type, use css for drop indicator instead of styles and fix tab indicators. * [top-level-tabs] add 'Collapse tab content' to delete tabs button * [dnd] add depth validation to drag and drop logic * [dashboard-builder] add resize action, enforce minimum width of columns, column children inherit column size when necessary, meta.rowStyle => meta.background, add background to columns * [dashboard-builder] make sure getChildWidth returns a number

* [dashboard-builder] remove spacer component * [dashboard-builder] better transparent indicator, better grid gutter logic, no dragging top-level tabs, headers are multiples of grid unit, fix row height granularity, update redux state key dashboard => dashboardLayout * [dashboard-builder] don't blast column child dimensions on resize * [dashboard-builder] ResizableContainer min size can't be smaller than size, fix row style, role=none on WithPopoverMenu container * [edit mode] add edit mode to redux and propogate to all <DashboardComponent />s * [toasts] add Toast component, ToastPresenter container and component, and toast redux actions + reducers * [dashboard-builder] add info toast when dropResult overflows parent

williaster · 2018-04-18T21:31:09Z

superset/assets/javascripts/dashboard/actions/dashboard.js

@@ -0,0 +1,165 @@
+import $ from 'jquery';


should this file be called dashboardState?

williaster · 2018-04-18T22:22:27Z

superset/assets/javascripts/dashboard/actions/sliceEntities.js

@@ -0,0 +1,91 @@
+/* global notify */


should we update this to use the new toast actions? can be in a future PR if you want.

williaster · 2018-04-18T22:23:09Z

superset/assets/javascripts/dashboard/actions/sliceEntities.js

+              slice_name: slice.slice_name,
+              edit_url: slice.edit_url,
+              form_data,
+              datasource: form_data.datasource,


why do we copy this if the slice already has form_data?

williaster · 2018-04-18T22:24:51Z

superset/assets/javascripts/dashboard/components/Dashboard.jsx

+  slicePropShape,
+  dashboardInfoPropShape,
+  dashboardStatePropShape,
+} from '../v2/util/propShapes';


williaster · 2018-04-18T22:28:47Z

superset/assets/javascripts/dashboard/components/SliceAdder.jsx


 const propTypes = {
-  dashboard: PropTypes.object.isRequired,
-  triggerNode: PropTypes.node.isRequired,
+  actions: PropTypes.object,


it'd be great to move away from this pattern of not defining explicit action shapes, can be error prone and harder to refactor than it should be. not a blocker for this but fyi.

williaster · 2018-04-18T22:33:50Z

superset/assets/javascripts/dashboard/index.jsx

-
-import emptyDashboardLayout from './v2/fixtures/emptyDashboardLayout';
-import rootReducer from './v2/reducers/';
+import getInitialState from './reducers/initState';


nit -- I would call this getInitialState, it's not clear it's a function (vs an object) and there's no benefit in saving 3 characters by calling it init instead of initial

but love having it in its own file! 💯

williaster · 2018-04-18T22:35:21Z

superset/assets/javascripts/dashboard/reducers/dashboard.js

@@ -0,0 +1,115 @@
+import { merge as mergeArray } from 'd3';
+
+import * as actions from '../actions/dashboard';


I would import all of these individually. also, should this file (similar to actions) be called dashboardState?

williaster · 2018-04-18T22:36:17Z

superset/assets/javascripts/dashboard/reducers/index.js

+import dashboardState from './dashboard';
+import datasources from './datasources';
+import sliceEntities from './sliceEntities';
+import dashboardLayout from '../v2/reducers/index';


should you update the /v2/reducers/index file to be called dashboardLayout?

dashboardLayout is basic layout reducer. index.js is wrapping dashboardLayout with undoable HOC.

williaster · 2018-04-18T22:36:33Z

superset/assets/javascripts/dashboard/reducers/index.js

+import shortid from 'shortid';
+
+import charts from '../../chart/chartReducer';
+import dashboardState from './dashboard';


I'd update the file to be called dashboardState too

williaster · 2018-04-18T22:37:14Z

superset/assets/javascripts/dashboard/reducers/initState.js

+import { getColorFromScheme } from '../../modules/colors';
+import layoutConverter from '../util/dashboardLayoutConverter';
+
+export default function(bootstrapData) {


I think you'll get some .eslint errors for this soon, but debugging is easier if you name your functions

fixed lint.

williaster · 2018-04-18T22:39:30Z

superset/assets/javascripts/dashboard/reducers/initState.js

+  delete dashboard.position_json;
+  delete dashboard.css;
+
+  // charts: dynamic queries


can you just rename these to chartQueries and sliceEntities so you wouldn't need a comment? you could then just set the return object to chart: chartQueries

williaster · 2018-04-18T22:40:17Z

superset/assets/javascripts/dashboard/reducers/initState.js

+    sliceIds.add(key);
+  });
+
+  return {


<3 how readable this is now!

williaster · 2018-04-18T22:40:45Z

superset/assets/javascripts/dashboard/reducers/sliceEntities.js

@@ -0,0 +1,57 @@
+import * as actions from '../actions/sliceEntities';


nit -- just import actual functions

williaster · 2018-04-18T22:42:25Z

superset/assets/javascripts/dashboard/util/dashboardLayoutConverter.js

+      }
+    }
+
+    rowContainer.meta.width = getChildrenSum(rowContainer.children, 'width', root);


weren't we going to not set these on rows?

williaster · 2018-04-18T22:42:49Z

superset/assets/javascripts/dashboard/util/dashboardLayoutConverter.js

+
+            // add col meta
+            colContainer.meta.width = getChildrenMax(colContainer.children, 'width', root);
+            colContainer.meta.height = getChildrenSum(colContainer.children, 'height', root);


columns should only have widths, yeah?

i removed height calculation from row and column. width might be used by row and column that are nested in column. but at the end of function i remove row's meta.width.

williaster · 2018-04-18T23:06:25Z

superset/assets/stylesheets/dashboard.less

+
+.component-layer {
+  .new-component.static {
+    cursor: pointer;


if it's static should the pointer be initial?

.new-component has cursor: move. .static mark it not drag-able, and this one is a link, click to open slices list.

williaster · 2018-04-18T23:06:53Z

superset/assets/stylesheets/dashboard.less

+  color: orange;
+}
+
+.dashboard-builder-sidepane {


it'd be great if you could help make the css more modular and move all builder sidepane css to it's own builder-sidepane.less file. this is what I did for all of the new v2 components. large css files bloat over time.

graceguo-supercat · 2018-04-24T17:51:36Z

closed. code is fixed and merged in #4849

williaster and others added 9 commits March 5, 2018 16:01

fix rebase error, clean up css organization and use @less vars

2c6d610

define dashboard redux state

4b99906

update dashobard state reducer

677b50a

dashboard layout converter + grid render

9d91000

builder pane + slice adder

c05db5a

Dashboard header + slice header controls

df45e72

williaster reviewed Apr 18, 2018

View reviewed changes

superset/assets/javascripts/dashboard/reducers/initState.js

sliceIds.add(key);

});

return {

Copy link

Contributor

williaster Apr 18, 2018

Choose a reason for hiding this comment

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

<3 how readable this is now!

williaster reviewed Apr 18, 2018

View reviewed changes

williaster force-pushed the dashboard-builder branch from a19a260 to 5f29bfd Compare April 19, 2018 00:04

graceguo-supercat closed this Apr 24, 2018

graceguo-supercat deleted the dashboard-builder-1st-comments branch June 11, 2020 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboard builder: fixed 1st round comments #4839

Dashboard builder: fixed 1st round comments #4839

graceguo-supercat commented Apr 18, 2018 •

edited

Loading

williaster Apr 18, 2018

graceguo-supercat Apr 19, 2018

williaster Apr 18, 2018

graceguo-supercat Apr 19, 2018

williaster Apr 18, 2018

williaster Apr 18, 2018

williaster Apr 18, 2018 •

edited

Loading

graceguo-supercat Apr 19, 2018

williaster Apr 18, 2018

williaster Apr 18, 2018

graceguo-supercat Apr 19, 2018

williaster Apr 18, 2018

graceguo-supercat Apr 19, 2018

williaster Apr 18, 2018

graceguo-supercat Apr 19, 2018

williaster Apr 18, 2018

graceguo-supercat Apr 19, 2018

williaster Apr 18, 2018

graceguo-supercat Apr 19, 2018

williaster Apr 18, 2018

graceguo-supercat Apr 19, 2018

williaster Apr 18, 2018

williaster Apr 18, 2018

graceguo-supercat Apr 19, 2018

williaster Apr 18, 2018

williaster Apr 18, 2018

graceguo-supercat Apr 19, 2018

williaster Apr 18, 2018

graceguo-supercat Apr 19, 2018

williaster Apr 18, 2018 •

edited

Loading

graceguo-supercat Apr 19, 2018

graceguo-supercat commented Apr 24, 2018

		@@ -0,0 +1,115 @@
		import { merge as mergeArray } from 'd3';

		import * as actions from '../actions/dashboard';

		@@ -0,0 +1,57 @@
		import * as actions from '../actions/sliceEntities';

Dashboard builder: fixed 1st round comments #4839

Dashboard builder: fixed 1st round comments #4839

Conversation

graceguo-supercat commented Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat commented Apr 24, 2018

graceguo-supercat commented Apr 18, 2018 •

edited

Loading

williaster Apr 18, 2018 •

edited

Loading

williaster Apr 18, 2018 •

edited

Loading