From 37e27e7b8442523368517967cd448e9328ecec15 Mon Sep 17 00:00:00 2001 From: Jen Jones Arnesen Date: Wed, 23 Sep 2020 09:17:55 +0200 Subject: [PATCH 1/4] fix: if editing then use the original vis type --- src/components/Item/VisualizationItem/Item.js | 31 +++++++++++-------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/components/Item/VisualizationItem/Item.js b/src/components/Item/VisualizationItem/Item.js index c0e374b0b..3ae33cd92 100644 --- a/src/components/Item/VisualizationItem/Item.js +++ b/src/components/Item/VisualizationItem/Item.js @@ -14,6 +14,7 @@ import LoadingMask from './LoadingMask' import * as pluginManager from './plugin' import { sGetVisualization } from '../../../reducers/visualizations' +import { sGetIsEditing } from '../../../reducers/editDashboard' import { sGetItemFiltersRoot, DEFAULT_STATE_ITEM_FILTERS, @@ -253,13 +254,17 @@ export class Item extends Component { ) } - onSelectActiveType = type => { + selectActiveType = type => { type !== this.getActiveType() && - this.props.onSelectActiveType(this.props.visualization.id, type) + this.props.selectActiveType(this.props.visualization.id, type) } - getActiveType = () => - this.props.visualization.activeType || this.props.item.type + getActiveType = () => { + if (this.props.isEditing) { + return this.props.item.type + } + return this.props.visualization.activeType || this.props.item.type + } pluginIsAvailable = () => pluginManager.pluginIsAvailable( @@ -283,7 +288,7 @@ export class Item extends Component { { : DEFAULT_STATE_ITEM_FILTERS return { + isEditing: sGetIsEditing(state), itemFilters, visualization: sGetVisualization( state, @@ -352,11 +359,9 @@ const mapStateToProps = (state, ownProps) => { } } -const mapDispatchToProps = dispatch => ({ - onVisualizationLoaded: visualization => - dispatch(acAddVisualization(visualization)), - onSelectActiveType: (id, type, activeType) => - dispatch(acSetActiveVisualizationType(id, type, activeType)), -}) +const mapDispatchToProps = { + selectActiveType: acSetActiveVisualizationType, + updateVisualization: acAddVisualization, +} export default connect(mapStateToProps, mapDispatchToProps)(Item) From c36d7a81f0f14db65e55a96a21dd563cf7736fb9 Mon Sep 17 00:00:00 2001 From: Jen Jones Arnesen Date: Wed, 23 Sep 2020 09:25:00 +0200 Subject: [PATCH 2/4] fix: rename --- src/components/Item/VisualizationItem/Item.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Item/VisualizationItem/Item.js b/src/components/Item/VisualizationItem/Item.js index 3ae33cd92..462fe33ba 100644 --- a/src/components/Item/VisualizationItem/Item.js +++ b/src/components/Item/VisualizationItem/Item.js @@ -65,7 +65,7 @@ export class Item extends Component { } async componentDidMount() { - this.props.onVisualizationLoaded( + this.props.updateVisualization( // TODO do not call fetch on the pluginManager, do it here as the manager will eventually be removed... await pluginManager.fetch(this.props.item) ) From b12f501d21deff08c7c77526e46b6d625163c78d Mon Sep 17 00:00:00 2001 From: Jen Jones Arnesen Date: Wed, 23 Sep 2020 14:06:32 +0200 Subject: [PATCH 3/4] fix: store active type separately from vis and clear everything on switch db --- src/actions/selected.js | 28 +++++++++++++++---- src/actions/visualizations.js | 14 +++------- .../Item/VisualizationItem/DefaultPlugin.js | 27 ++++-------------- src/components/Item/VisualizationItem/Item.js | 17 +++++------ src/reducers/selected.js | 27 ++++++++++++++++++ src/reducers/visualizations.js | 14 ++-------- 6 files changed, 72 insertions(+), 55 deletions(-) diff --git a/src/actions/selected.js b/src/actions/selected.js index 818ab5177..b2c22de64 100644 --- a/src/actions/selected.js +++ b/src/actions/selected.js @@ -3,6 +3,8 @@ import { SET_SELECTED_ID, SET_SELECTED_ISLOADING, SET_SELECTED_SHOWDESCRIPTION, + SET_SELECTED_ITEM_ACTIVE_TYPE, + CLEAR_SELECTED_ITEM_ACTIVE_TYPES, sGetSelectedIsLoading, sGetSelectedId, } from '../reducers/selected' @@ -12,7 +14,7 @@ import { acSetDashboardItems, acAppendDashboards } from './dashboards' import { acClearItemFilters } from './itemFilters' import { tGetMessages } from '../components/Item/MessagesItem/actions' import { acReceivedSnackbarMessage, acCloseSnackbar } from './snackbar' -import { acAddVisualization } from './visualizations' +import { acAddVisualization, acClearVisualizations } from './visualizations' import { apiFetchDashboard } from '../api/dashboards' import { storePreferredDashboardId } from '../api/localStorage' @@ -52,6 +54,20 @@ export const acSetSelectedShowDescription = value => ({ value, }) +export const acSetSelectedItemActiveType = (id, activeType) => { + const action = { + type: SET_SELECTED_ITEM_ACTIVE_TYPE, + id, + activeType, + } + + return action +} + +export const acClearSelectedItemActiveTypes = () => ({ + type: CLEAR_SELECTED_ITEM_ACTIVE_TYPES, +}) + // thunks export const tSetSelectedDashboardById = id => async (dispatch, getState) => { dispatch(acSetSelectedIsLoading(true)) @@ -80,6 +96,12 @@ export const tSetSelectedDashboardById = id => async (dispatch, getState) => { storePreferredDashboardId(sGetUserUsername(getState()), id) + if (id !== sGetSelectedId(getState())) { + dispatch(acClearItemFilters()) + dispatch(acClearVisualizations()) + dispatch(acClearSelectedItemActiveTypes()) + } + customDashboard.dashboardItems.forEach(item => { switch (item.type) { case REPORT_TABLE: @@ -97,10 +119,6 @@ export const tSetSelectedDashboardById = id => async (dispatch, getState) => { } }) - if (id !== sGetSelectedId(getState())) { - dispatch(acClearItemFilters()) - } - dispatch(acSetSelectedId(id)) dispatch(acSetSelectedIsLoading(false)) diff --git a/src/actions/visualizations.js b/src/actions/visualizations.js index d00979593..ee1652a79 100644 --- a/src/actions/visualizations.js +++ b/src/actions/visualizations.js @@ -1,6 +1,6 @@ import { ADD_VISUALIZATION, - SET_ACTIVE_VISUALIZATION_TYPE, + CLEAR_VISUALIZATIONS, } from '../reducers/visualizations' export const acAddVisualization = value => ({ @@ -8,12 +8,6 @@ export const acAddVisualization = value => ({ value, }) -export const acSetActiveVisualizationType = (id, activeType) => { - const action = { - type: SET_ACTIVE_VISUALIZATION_TYPE, - id, - activeType, - } - - return action -} +export const acClearVisualizations = () => ({ + type: CLEAR_VISUALIZATIONS, +}) diff --git a/src/components/Item/VisualizationItem/DefaultPlugin.js b/src/components/Item/VisualizationItem/DefaultPlugin.js index eb28df41c..e6afbdf35 100644 --- a/src/components/Item/VisualizationItem/DefaultPlugin.js +++ b/src/components/Item/VisualizationItem/DefaultPlugin.js @@ -48,25 +48,15 @@ class DefaultPlugin extends Component { ) && this.shouldPluginReload(prevProps) ) { - const prevVis = orObject(prevProps.visualization) - const currentVis = this.props.visualization - - const useActiveType = - currentVis.activeType !== prevVis.activeType || - currentVis.activeType !== this.props.item.type - if ( - useActiveType || + this.props.activeType !== prevProps.activeType || this.props.itemFilters !== prevProps.itemFilters ) { - pluginManager.unmount( - this.props.item, - prevVis.activeType || this.props.item.type - ) + pluginManager.unmount(this.props.item, prevProps.activeType) pluginManager.load(this.props.item, this.props.visualization, { credentials: this.pluginCredentials, - activeType: useActiveType ? currentVis.activeType : null, + activeType: this.props.activeType, }) } } @@ -83,9 +73,7 @@ class DefaultPlugin extends Component { ) { pluginManager.load(this.props.item, this.props.visualization, { credentials: this.pluginCredentials, - activeType: this.props.useActiveType - ? this.getActiveType() - : null, + activeType: this.props.activeType, options: this.props.options, }) } @@ -102,13 +90,10 @@ class DefaultPlugin extends Component { this.props.visualization ) ) { - pluginManager.unmount(this.props.item, this.getActiveType()) + pluginManager.unmount(this.props.item, this.props.activeType) } } - getActiveType = () => - this.props.visualization.activeType || this.props.item.type - render() { const { classes, item, visualization, style } = this.props const pluginIsAvailable = pluginManager.pluginIsAvailable( @@ -131,12 +116,12 @@ DefaultPlugin.contextTypes = { } DefaultPlugin.propTypes = { + activeType: PropTypes.string, classes: PropTypes.object, item: PropTypes.object, itemFilters: PropTypes.object, options: PropTypes.object, style: PropTypes.object, - useActiveType: PropTypes.bool, visualization: PropTypes.object, } diff --git a/src/components/Item/VisualizationItem/Item.js b/src/components/Item/VisualizationItem/Item.js index 462fe33ba..4376fd7cb 100644 --- a/src/components/Item/VisualizationItem/Item.js +++ b/src/components/Item/VisualizationItem/Item.js @@ -14,15 +14,14 @@ import LoadingMask from './LoadingMask' import * as pluginManager from './plugin' import { sGetVisualization } from '../../../reducers/visualizations' +import { sGetSelectedItemActiveType } from '../../../reducers/selected' import { sGetIsEditing } from '../../../reducers/editDashboard' import { sGetItemFiltersRoot, DEFAULT_STATE_ITEM_FILTERS, } from '../../../reducers/itemFilters' -import { - acAddVisualization, - acSetActiveVisualizationType, -} from '../../../actions/visualizations' +import { acAddVisualization } from '../../../actions/visualizations' +import { acSetSelectedItemActiveType } from '../../../actions/selected' import { VISUALIZATION, MAP, @@ -156,7 +155,7 @@ export class Item extends Component { const props = { ...this.props, - useActiveType: !isEditMode(this.props.dashboardMode), + activeType, visualization, classes, style: this.memoizedGetContentStyle( @@ -256,14 +255,14 @@ export class Item extends Component { selectActiveType = type => { type !== this.getActiveType() && - this.props.selectActiveType(this.props.visualization.id, type) + this.props.selectActiveType(this.props.item.id, type) } getActiveType = () => { if (this.props.isEditing) { return this.props.item.type } - return this.props.visualization.activeType || this.props.item.type + return this.props.activeType || this.props.item.type } pluginIsAvailable = () => @@ -328,6 +327,7 @@ Item.contextTypes = { } Item.propTypes = { + activeType: PropTypes.string, dashboardMode: PropTypes.string, isEditing: PropTypes.bool, item: PropTypes.object, @@ -350,6 +350,7 @@ const mapStateToProps = (state, ownProps) => { : DEFAULT_STATE_ITEM_FILTERS return { + activeType: sGetSelectedItemActiveType(state, ownProps.item?.id), isEditing: sGetIsEditing(state), itemFilters, visualization: sGetVisualization( @@ -360,7 +361,7 @@ const mapStateToProps = (state, ownProps) => { } const mapDispatchToProps = { - selectActiveType: acSetActiveVisualizationType, + selectActiveType: acSetSelectedItemActiveType, updateVisualization: acAddVisualization, } diff --git a/src/reducers/selected.js b/src/reducers/selected.js index 32c304e46..815d3e51c 100644 --- a/src/reducers/selected.js +++ b/src/reducers/selected.js @@ -6,10 +6,14 @@ import { validateReducer } from '../modules/util' export const SET_SELECTED_ID = 'SET_SELECTED_ID' export const SET_SELECTED_ISLOADING = 'SET_SELECTED_ISLOADING' export const SET_SELECTED_SHOWDESCRIPTION = 'SET_SELECTED_SHOWDESCRIPTION' +export const SET_SELECTED_ITEM_ACTIVE_TYPE = 'SET_SELECTED_ITEM_ACTIVE_TYPE' +export const CLEAR_SELECTED_ITEM_ACTIVE_TYPES = + 'CLEAR_SELECTED_ITEM_ACTIVE_TYPES' export const DEFAULT_STATE_SELECTED_ID = null export const DEFAULT_STATE_SELECTED_ISLOADING = false export const DEFAULT_STATE_SELECTED_SHOWDESCRIPTION = false +export const DEFAULT_STATE_SELECTED_ITEM_ACTIVE_TYPES = {} export const NON_EXISTING_DASHBOARD_ID = '0' @@ -55,10 +59,30 @@ const showDescription = ( } } +const itemActiveTypes = ( + state = DEFAULT_STATE_SELECTED_ITEM_ACTIVE_TYPES, + action +) => { + switch (action.type) { + case SET_SELECTED_ITEM_ACTIVE_TYPE: { + return { + ...state, + [action.id]: action.activeType, + } + } + case CLEAR_SELECTED_ITEM_ACTIVE_TYPES: { + return DEFAULT_STATE_SELECTED_ITEM_ACTIVE_TYPES + } + default: + return state + } +} + export default combineReducers({ id, isLoading, showDescription, + itemActiveTypes, }) // Selectors @@ -71,3 +95,6 @@ export const sGetSelectedIsLoading = state => sGetSelectedRoot(state).isLoading export const sGetSelectedShowDescription = state => sGetSelectedRoot(state).showDescription + +export const sGetSelectedItemActiveType = (state, id) => + sGetSelectedRoot(state).itemActiveTypes[id] diff --git a/src/reducers/visualizations.js b/src/reducers/visualizations.js index 09ea6c95c..937bc69b0 100644 --- a/src/reducers/visualizations.js +++ b/src/reducers/visualizations.js @@ -5,6 +5,7 @@ import objectClean from 'd2-utilizr/lib/objectClean' export const ADD_VISUALIZATION = 'ADD_VISUALIZATION' export const SET_ACTIVE_VISUALIZATION_TYPE = 'SET_ACTIVE_VISUALIZATION_TYPE' +export const CLEAR_VISUALIZATIONS = 'CLEAR_VISUALIZATIONS' export const DEFAULT_STATE_VISUALIZATIONS = {} @@ -24,17 +25,8 @@ export default (state = DEFAULT_STATE_VISUALIZATIONS, action) => { ), } } - case SET_ACTIVE_VISUALIZATION_TYPE: { - return { - ...state, - [action.id]: objectClean( - { - ...orObject(state[action.id]), - activeType: action.activeType, - }, - isEmpty - ), - } + case CLEAR_VISUALIZATIONS: { + return DEFAULT_STATE_VISUALIZATIONS } default: return state From 34f9370141303bf15410456e6e8c361de8db88c2 Mon Sep 17 00:00:00 2001 From: Jen Jones Arnesen Date: Thu, 24 Sep 2020 08:23:22 +0200 Subject: [PATCH 4/4] fix: test --- src/reducers/__tests__/selected.spec.js | 102 +++++++++++++----- src/reducers/__tests__/visualizations.spec.js | 61 +++++------ 2 files changed, 101 insertions(+), 62 deletions(-) diff --git a/src/reducers/__tests__/selected.spec.js b/src/reducers/__tests__/selected.spec.js index 208259d3d..fc235525e 100644 --- a/src/reducers/__tests__/selected.spec.js +++ b/src/reducers/__tests__/selected.spec.js @@ -2,6 +2,9 @@ import reducer, { SET_SELECTED_ID, SET_SELECTED_ISLOADING, SET_SELECTED_SHOWDESCRIPTION, + SET_SELECTED_ITEM_ACTIVE_TYPE, + CLEAR_SELECTED_ITEM_ACTIVE_TYPES, + DEFAULT_STATE_SELECTED_ITEM_ACTIVE_TYPES, } from '../selected' describe('selected dashboard reducer', () => { @@ -9,47 +12,88 @@ describe('selected dashboard reducer', () => { id: null, isLoading: false, showDescription: false, + itemActiveTypes: {}, } - describe('reducer', () => { - it('should set the selected dashboard id', () => { - const id = 'my favorite dashboard' - const expectedState = Object.assign({}, defaultState, { id }) + it('sets the selected dashboard id', () => { + const id = 'my favorite dashboard' + const expectedState = Object.assign({}, defaultState, { id }) - const actualState = reducer(defaultState, { - type: SET_SELECTED_ID, - value: id, - }) + const actualState = reducer(defaultState, { + type: SET_SELECTED_ID, + value: id, + }) + + expect(actualState).toEqual(expectedState) + }) - expect(actualState).toEqual(expectedState) + it('sets the selected dashboard isLoading state', () => { + const isLoading = true + const expectedState = Object.assign({}, defaultState, { + isLoading, }) - it('should set the selected dashboard isLoading state', () => { - const isLoading = true - const expectedState = Object.assign({}, defaultState, { - isLoading, - }) + const actualState = reducer(defaultState, { + type: SET_SELECTED_ISLOADING, + value: isLoading, + }) - const actualState = reducer(defaultState, { - type: SET_SELECTED_ISLOADING, - value: isLoading, - }) + expect(actualState).toEqual(expectedState) + }) - expect(actualState).toEqual(expectedState) + it('sets the selected dashboard showDescription state', () => { + const showDescription = true + const expectedState = Object.assign({}, defaultState, { + showDescription, }) - it('should set the selected dashboard showDescription state', () => { - const showDescription = true - const expectedState = Object.assign({}, defaultState, { - showDescription, - }) + const actualState = reducer(defaultState, { + type: SET_SELECTED_SHOWDESCRIPTION, + value: showDescription, + }) - const actualState = reducer(defaultState, { - type: SET_SELECTED_SHOWDESCRIPTION, - value: showDescription, - }) + expect(actualState).toEqual(expectedState) + }) + + it('sets an item active Type', () => { + const action = { + type: SET_SELECTED_ITEM_ACTIVE_TYPE, + id: 'rainbowDash', + activeType: 'PONY', + } - expect(actualState).toEqual(expectedState) + const expectedState = Object.assign({}, defaultState, { + itemActiveTypes: { rainbowDash: 'PONY' }, }) + + const actualState = reducer(defaultState, action) + + expect(actualState).toEqual(expectedState) + }) + + it('clears the item active types', () => { + const action = { + type: SET_SELECTED_ITEM_ACTIVE_TYPE, + id: 'rainbowDash', + activeType: 'PONY', + } + + const expectedState = Object.assign({}, defaultState, { + itemActiveTypes: { rainbowDash: 'PONY' }, + }) + + const actualState = reducer(defaultState, action) + + expect(actualState).toEqual(expectedState) + + const updatedState = reducer(actualState, { + type: CLEAR_SELECTED_ITEM_ACTIVE_TYPES, + }) + + expect(updatedState).toEqual( + Object.assign({}, defaultState, { + itemActiveTypes: DEFAULT_STATE_SELECTED_ITEM_ACTIVE_TYPES, + }) + ) }) }) diff --git a/src/reducers/__tests__/visualizations.spec.js b/src/reducers/__tests__/visualizations.spec.js index 01a08b0b7..8b38b1313 100644 --- a/src/reducers/__tests__/visualizations.spec.js +++ b/src/reducers/__tests__/visualizations.spec.js @@ -1,73 +1,68 @@ import reducer, { DEFAULT_STATE_VISUALIZATIONS, ADD_VISUALIZATION, - SET_ACTIVE_VISUALIZATION_TYPE, + CLEAR_VISUALIZATIONS, } from '../visualizations' describe('visualizations reducer', () => { - const activeType = 'CHART' - const visualization = { - id: 'abc', - name: 'funny name', - } - - const visualizationWithActiveType = { - ...visualization, - activeType, + id: 'rainbowDash', + name: 'Rainbow Dash', } const state = { [visualization.id]: visualization, } - const stateWithActiveType = { - [visualization.id]: visualizationWithActiveType, - } - it('should return the default state', () => { - const actualState = reducer(undefined, { type: 'NO_MATCH' }) + const actualState = reducer(undefined, {}) const expectedState = DEFAULT_STATE_VISUALIZATIONS expect(actualState).toEqual(expectedState) }) - it('should add a visualization (ADD_VISUALIZATION)', () => { + it('adds a visualization (ADD_VISUALIZATION)', () => { const action = { type: ADD_VISUALIZATION, value: visualization, } const actualState = reducer(DEFAULT_STATE_VISUALIZATIONS, action) - const expectedState = state - expect(actualState).toEqual(expectedState) + expect(actualState).toEqual(state) }) - it('should update a visualization with activeType (SET_ACTIVE_VISUALIZATION_TYPE)', () => { + it('updates a visualization', () => { const action = { - type: SET_ACTIVE_VISUALIZATION_TYPE, - id: visualization.id, - activeType, + type: ADD_VISUALIZATION, + value: visualization, } - const currentState = state - const expectedState = stateWithActiveType - const actualState = reducer(currentState, action) + const newState = reducer(undefined, action) + expect(newState).toEqual(state) - expect(actualState).toEqual(expectedState) + const value = Object.assign({}, visualization, { age: 10 }) + + const updatedState = reducer(newState, { + type: ADD_VISUALIZATION, + value, + }) + + expect(updatedState).toEqual({ + rainbowDash: { id: 'rainbowDash', name: 'Rainbow Dash', age: 10 }, + }) }) - it('should update a visualization with removed activeType (SET_ACTIVE_VISUALIZATION_TYPE)', () => { + it('clears the visualizations', () => { const action = { - type: SET_ACTIVE_VISUALIZATION_TYPE, - id: visualization.id, + type: ADD_VISUALIZATION, + value: visualization, } - const currentState = stateWithActiveType - const expectedState = state - const actualState = reducer(currentState, action) + const newState = reducer(undefined, action) + expect(newState).toEqual(state) - expect(actualState).toEqual(expectedState) + const updatedState = reducer(newState, { type: CLEAR_VISUALIZATIONS }) + expect(updatedState).toEqual(DEFAULT_STATE_VISUALIZATIONS) }) })