[Lyft-GA] Enable color consistency in a dashboard (#7135)

* Enable color consistency in a dashboard Moved actions, minor UI, allowed dashboard copy Fix linting errors Undo unintentional change Updated and added unit tests Fail quietly if package has not been updated Fail quietly on dashboard copy if package is old * Update packages * Remove unnecessary code * Addressed Grace's comments * Small fix for item key * Reset chart's color during exploration * Do not reset chart form data when exploring chart
apache · Mar 31, 2019 · 4a00e8e · 4a00e8e
1 parent adfff5d
commit 4a00e8e
Show file tree

Hide file tree

Showing 28 changed files with 586 additions and 170 deletions.
diff --git a/superset/assets/package-lock.json b/superset/assets/package-lock.json
diff --git a/superset/assets/spec/javascripts/dashboard/components/DashboardBuilder_spec.jsx b/superset/assets/spec/javascripts/dashboard/components/DashboardBuilder_spec.jsx
@@ -31,6 +31,7 @@ import DashboardComponent from '../../../../src/dashboard/containers/DashboardCo
 import DashboardHeader from '../../../../src/dashboard/containers/DashboardHeader';
 import DashboardGrid from '../../../../src/dashboard/containers/DashboardGrid';
 import * as dashboardStateActions from '../../../../src/dashboard/actions/dashboardState';
+import { BUILDER_PANE_TYPE } from '../../../../src/dashboard/util/constants';
 
 import WithDragDropContext from '../helpers/WithDragDropContext';
 import {
@@ -61,7 +62,10 @@ describe('DashboardBuilder', () => {
     dashboardLayout,
     deleteTopLevelTabs() {},
     editMode: false,
-    showBuilderPane: false,
+    showBuilderPane() {},
+    builderPaneType: BUILDER_PANE_TYPE.NONE,
+    setColorSchemeAndUnsavedChanges() {},
+    colorScheme: undefined,
     handleComponentDrop() {},
     toggleBuilderPane() {},
   };
@@ -143,11 +147,27 @@ describe('DashboardBuilder', () => {
     expect(parentSize.find(DashboardGrid)).toHaveLength(expectedCount);
   });
 
-  it('should render a BuilderComponentPane if editMode=showBuilderPane=true', () => {
+  it('should render a BuilderComponentPane if editMode=true and user selects "Insert Components" pane', () => {
     const wrapper = setup();
     expect(wrapper.find(BuilderComponentPane)).toHaveLength(0);
 
-    wrapper.setProps({ ...props, editMode: true, showBuilderPane: true });
+    wrapper.setProps({
+      ...props,
+      editMode: true,
+      builderPaneType: BUILDER_PANE_TYPE.ADD_COMPONENTS,
+    });
+    expect(wrapper.find(BuilderComponentPane)).toHaveLength(1);
+  });
+
+  it('should render a BuilderComponentPane if editMode=true and user selects "Colors" pane', () => {
+    const wrapper = setup();
+    expect(wrapper.find(BuilderComponentPane)).toHaveLength(0);
+
+    wrapper.setProps({
+      ...props,
+      editMode: true,
+      builderPaneType: BUILDER_PANE_TYPE.COLORS,
+    });
     expect(wrapper.find(BuilderComponentPane)).toHaveLength(1);
   });
 

diff --git a/superset/assets/spec/javascripts/dashboard/components/Header_spec.jsx b/superset/assets/spec/javascripts/dashboard/components/Header_spec.jsx
@@ -24,6 +24,7 @@ import FaveStar from '../../../../src/components/FaveStar';
 import HeaderActionsDropdown from '../../../../src/dashboard/components/HeaderActionsDropdown';
 import Button from '../../../../src/components/Button';
 import UndoRedoKeylisteners from '../../../../src/dashboard/components/UndoRedoKeylisteners';
+import { BUILDER_PANE_TYPE } from '../../../../src/dashboard/util/constants';
 
 describe('Header', () => {
   const props = {
@@ -46,7 +47,8 @@ describe('Header', () => {
     updateDashboardTitle: () => {},
     editMode: false,
     setEditMode: () => {},
-    showBuilderPane: false,
+    showBuilderPane: () => {},
+    builderPaneType: BUILDER_PANE_TYPE.NONE,
     toggleBuilderPane: () => {},
     updateCss: () => {},
     hasUnsavedChanges: false,
@@ -150,9 +152,9 @@ describe('Header', () => {
       expect(wrapper.find(HeaderActionsDropdown)).toHaveLength(1);
     });
 
-    it('should render four Buttons', () => {
+    it('should render five Buttons', () => {
       const wrapper = setup(overrideProps);
-      expect(wrapper.find(Button)).toHaveLength(4);
+      expect(wrapper.find(Button)).toHaveLength(5);
     });
 
     it('should set up undo/redo', () => {

diff --git a/superset/assets/spec/javascripts/dashboard/fixtures/mockDashboardState.js b/superset/assets/spec/javascripts/dashboard/fixtures/mockDashboardState.js
@@ -17,14 +17,15 @@
  * under the License.
  */
 import { id as sliceId } from './mockChartQueries';
+import { BUILDER_PANE_TYPE } from '../../../../src/dashboard/util/constants';
 
 export default {
   sliceIds: [sliceId],
   refresh: false,
   filters: {},
   expandedSlices: {},
   editMode: false,
-  showBuilderPane: false,
+  builderPaneType: BUILDER_PANE_TYPE.NONE,
   hasUnsavedChanges: false,
   maxUndoHistoryExceeded: false,
   isStarred: true,

diff --git a/superset/assets/spec/javascripts/dashboard/reducers/dashboardState_spec.js b/superset/assets/spec/javascripts/dashboard/reducers/dashboardState_spec.js
@@ -25,12 +25,12 @@ import {
   SET_EDIT_MODE,
   SET_MAX_UNDO_HISTORY_EXCEEDED,
   SET_UNSAVED_CHANGES,
-  TOGGLE_BUILDER_PANE,
   TOGGLE_EXPAND_SLICE,
   TOGGLE_FAVE_STAR,
 } from '../../../../src/dashboard/actions/dashboardState';
 
 import dashboardStateReducer from '../../../../src/dashboard/reducers/dashboardState';
+import { BUILDER_PANE_TYPE } from '../../../../src/dashboard/util/constants';
 
 describe('dashboardState reducer', () => {
   it('should return initial state', () => {
@@ -79,23 +79,10 @@ describe('dashboardState reducer', () => {
         { editMode: false },
         { type: SET_EDIT_MODE, editMode: true },
       ),
-    ).toEqual({ editMode: true, showBuilderPane: true });
-  });
-
-  it('should toggle builder pane', () => {
-    expect(
-      dashboardStateReducer(
-        { showBuilderPane: false },
-        { type: TOGGLE_BUILDER_PANE },
-      ),
-    ).toEqual({ showBuilderPane: true });
-
-    expect(
-      dashboardStateReducer(
-        { showBuilderPane: true },
-        { type: TOGGLE_BUILDER_PANE },
-      ),
-    ).toEqual({ showBuilderPane: false });
+    ).toEqual({
+      editMode: true,
+      builderPaneType: BUILDER_PANE_TYPE.ADD_COMPONENTS,
+    });
   });
 
   it('should toggle expanded slices', () => {
@@ -150,6 +137,8 @@ describe('dashboardState reducer', () => {
       hasUnsavedChanges: false,
       maxUndoHistoryExceeded: false,
       editMode: false,
+      builderPaneType: BUILDER_PANE_TYPE.NONE,
+      updatedColorScheme: false,
     });
   });
 

diff --git a/superset/assets/src/chart/ChartRenderer.jsx b/superset/assets/src/chart/ChartRenderer.jsx
@@ -87,7 +87,8 @@ class ChartRenderer extends React.Component {
         nextProps.height !== this.props.height ||
         nextProps.width !== this.props.width ||
         nextState.tooltip !== this.state.tooltip ||
-        nextProps.triggerRender) {
+        nextProps.triggerRender ||
+        nextProps.formData.color_scheme !== this.props.formData.color_scheme) {
         return true;
       }
     }

diff --git a/superset/assets/src/dashboard/actions/dashboardLayout.js b/superset/assets/src/dashboard/actions/dashboardLayout.js
@@ -209,7 +209,8 @@ export function undoLayoutAction() {
 
     if (
       dashboardLayout.past.length === 0 &&
-      !dashboardState.maxUndoHistoryExceeded
+      !dashboardState.maxUndoHistoryExceeded &&
+      !dashboardState.updatedColorScheme
     ) {
       dispatch(setUnsavedChanges(false));
     }

diff --git a/superset/assets/src/dashboard/actions/dashboardState.js b/superset/assets/src/dashboard/actions/dashboardState.js
@@ -225,9 +225,9 @@ export function startPeriodicRender(interval) {
   };
 }
 
-export const TOGGLE_BUILDER_PANE = 'TOGGLE_BUILDER_PANE';
-export function toggleBuilderPane() {
-  return { type: TOGGLE_BUILDER_PANE };
+export const SHOW_BUILDER_PANE = 'SHOW_BUILDER_PANE';
+export function showBuilderPane(builderPaneType) {
+  return { type: SHOW_BUILDER_PANE, builderPaneType };
 }
 
 export function addSliceToDashboard(id) {
@@ -266,6 +266,18 @@ export function removeSliceFromDashboard(id) {
   };
 }
 
+export const SET_COLOR_SCHEME = 'SET_COLOR_SCHEME';
+export function setColorScheme(colorScheme) {
+  return { type: SET_COLOR_SCHEME, colorScheme };
+}
+
+export function setColorSchemeAndUnsavedChanges(colorScheme) {
+  return dispatch => {
+    dispatch(setColorScheme(colorScheme));
+    dispatch(setUnsavedChanges(true));
+  };
+}
+
 // Undo history ---------------------------------------------------------------
 export const SET_MAX_UNDO_HISTORY_EXCEEDED = 'SET_MAX_UNDO_HISTORY_EXCEEDED';
 export function setMaxUndoHistoryExceeded(maxUndoHistoryExceeded = true) {

diff --git a/superset/assets/src/dashboard/components/BuilderComponentPane.jsx b/superset/assets/src/dashboard/components/BuilderComponentPane.jsx
@@ -19,49 +19,37 @@
 /* eslint-env browser */
 import PropTypes from 'prop-types';
 import React from 'react';
-import cx from 'classnames';
 import { StickyContainer, Sticky } from 'react-sticky';
 import { ParentSize } from '@vx/responsive';
-import { t } from '@superset-ui/translation';
 
-import NewColumn from './gridComponents/new/NewColumn';
-import NewDivider from './gridComponents/new/NewDivider';
-import NewHeader from './gridComponents/new/NewHeader';
-import NewRow from './gridComponents/new/NewRow';
-import NewTabs from './gridComponents/new/NewTabs';
-import NewMarkdown from './gridComponents/new/NewMarkdown';
-import SliceAdder from '../containers/SliceAdder';
-
-const SUPERSET_HEADER_HEIGHT = 59;
+import InsertComponentPane, {
+  SUPERSET_HEADER_HEIGHT,
+} from './InsertComponentPane';
+import ColorComponentPane from './ColorComponentPane';
+import { BUILDER_PANE_TYPE } from '../util/constants';
 
 const propTypes = {
   topOffset: PropTypes.number,
-  toggleBuilderPane: PropTypes.func.isRequired,
+  showBuilderPane: PropTypes.func.isRequired,
+  builderPaneType: PropTypes.string.isRequired,
+  setColorSchemeAndUnsavedChanges: PropTypes.func.isRequired,
+  colorScheme: PropTypes.string,
 };
 
 const defaultProps = {
   topOffset: 0,
+  colorScheme: undefined,
 };
 
 class BuilderComponentPane extends React.PureComponent {
-  constructor(props) {
-    super(props);
-    this.state = {
-      slideDirection: 'slide-out',
-    };
-
-    this.openSlicesPane = this.slide.bind(this, 'slide-in');
-    this.closeSlicesPane = this.slide.bind(this, 'slide-out');
-  }
-
-  slide(direction) {
-    this.setState({
-      slideDirection: direction,
-    });
-  }
-
   render() {
-    const { topOffset } = this.props;
+    const {
+      topOffset,
+      builderPaneType,
+      showBuilderPane,
+      setColorSchemeAndUnsavedChanges,
+      colorScheme,
+    } = this.props;
     return (
       <div
         className="dashboard-builder-sidepane"
@@ -78,56 +66,22 @@ class BuilderComponentPane extends React.PureComponent {
                     className="viewport"
                     style={isSticky ? { ...style, top: topOffset } : null}
                   >
-                    <div
-                      className={cx(
-                        'slider-container',
-                        this.state.slideDirection,
-                      )}
-                    >
-                      <div className="component-layer slide-content">
-                        <div className="dashboard-builder-sidepane-header">
-                          <span>{t('Insert components')}</span>
-                          <i
-                            className="fa fa-times trigger"
-                            onClick={this.props.toggleBuilderPane}
-                            role="none"
-                          />
-                        </div>
-                        <div
-                          className="new-component static"
-                          role="none"
-                          onClick={this.openSlicesPane}
-                        >
-                          <div className="new-component-placeholder fa fa-area-chart" />
-                          <div className="new-component-label">
-                            {t('Your charts & filters')}
-                          </div>
-
-                          <i className="fa fa-arrow-right trigger" />
-                        </div>
-                        <NewTabs />
-                        <NewRow />
-                        <NewColumn />
-                        <NewHeader />
-                        <NewMarkdown />
-                        <NewDivider />
-                      </div>
-                      <div className="slices-layer slide-content">
-                        <div
-                          className="dashboard-builder-sidepane-header"
-                          onClick={this.closeSlicesPane}
-                          role="none"
-                        >
-                          <i className="fa fa-arrow-left trigger" />
-                          <span>{t('Your charts and filters')}</span>
-                        </div>
-                        <SliceAdder
-                          height={
-                            height + (isSticky ? SUPERSET_HEADER_HEIGHT : 0)
-                          }
-                        />
-                      </div>
-                    </div>
+                    {builderPaneType === BUILDER_PANE_TYPE.ADD_COMPONENTS && (
+                      <InsertComponentPane
+                        height={height}
+                        isSticky={isSticky}
+                        showBuilderPane={showBuilderPane}
+                      />
+                    )}
+                    {builderPaneType === BUILDER_PANE_TYPE.COLORS && (
+                      <ColorComponentPane
+                        showBuilderPane={showBuilderPane}
+                        setColorSchemeAndUnsavedChanges={
+                          setColorSchemeAndUnsavedChanges
+                        }
+                        colorScheme={colorScheme}
+                      />
+                    )}
                   </div>
                 )}
               </Sticky>