From f7d3450b2478b4d33a3ed3e3312ded72d255bb70 Mon Sep 17 00:00:00 2001 From: Edoardo Sabadelli Date: Wed, 17 Mar 2021 16:14:06 +0100 Subject: [PATCH 1/5] refactor: remove unused action --- packages/app/src/actions/settings.js | 7 +----- .../src/reducers/__tests__/settings.spec.js | 22 +------------------ packages/app/src/reducers/settings.js | 4 ---- 3 files changed, 2 insertions(+), 31 deletions(-) diff --git a/packages/app/src/actions/settings.js b/packages/app/src/actions/settings.js index 0ec7155717..e12fb973c0 100644 --- a/packages/app/src/actions/settings.js +++ b/packages/app/src/actions/settings.js @@ -1,12 +1,7 @@ -import { SET_SETTINGS, ADD_SETTINGS } from '../reducers/settings' +import { ADD_SETTINGS } from '../reducers/settings' import { apiFetchSystemSettings } from '../api/settings' import { apiFetchOrganisationUnitRoots } from '@dhis2/analytics' -export const acSetSettings = value => ({ - type: SET_SETTINGS, - value, -}) - export const acAddSettings = value => ({ type: ADD_SETTINGS, value, diff --git a/packages/app/src/reducers/__tests__/settings.spec.js b/packages/app/src/reducers/__tests__/settings.spec.js index a7ffc8d048..7ae756c099 100644 --- a/packages/app/src/reducers/__tests__/settings.spec.js +++ b/packages/app/src/reducers/__tests__/settings.spec.js @@ -1,8 +1,4 @@ -import reducer, { - DEFAULT_SETTINGS, - SET_SETTINGS, - ADD_SETTINGS, -} from '../settings' +import reducer, { DEFAULT_SETTINGS, ADD_SETTINGS } from '../settings' describe('reducer: settings', () => { const currentState = { @@ -16,22 +12,6 @@ describe('reducer: settings', () => { expect(actualState).toEqual(DEFAULT_SETTINGS) }) - it(`${SET_SETTINGS}: should set settings`, () => { - const stateToSet = { - keyAnalysisRelativePeriod: 'LAST_4_QUARTERS', - keyAnalysisDisplayProperty: 'shortName', - } - - const actualState = reducer(currentState, { - type: SET_SETTINGS, - value: stateToSet, - }) - - const expectedState = stateToSet - - expect(actualState).toEqual(expectedState) - }) - it(`${ADD_SETTINGS}: should add settings`, () => { const stateToAdd = { keyAnalysisDisplayProperty: 'shortName', diff --git a/packages/app/src/reducers/settings.js b/packages/app/src/reducers/settings.js index b5c357f9fe..ae11b0320b 100644 --- a/packages/app/src/reducers/settings.js +++ b/packages/app/src/reducers/settings.js @@ -1,4 +1,3 @@ -export const SET_SETTINGS = 'SET_SETTINGS' export const ADD_SETTINGS = 'ADD_SETTINGS' export const DEFAULT_SETTINGS = { @@ -13,9 +12,6 @@ export const DEFAULT_SETTINGS = { export default (state = DEFAULT_SETTINGS, action) => { switch (action.type) { - case SET_SETTINGS: { - return Object.assign({}, action.value) - } case ADD_SETTINGS: { return { ...state, From a28ab76159cfaab72a5a107e9173c26ab294d90d Mon Sep 17 00:00:00 2001 From: Edoardo Sabadelli Date: Wed, 17 Mar 2021 16:15:00 +0100 Subject: [PATCH 2/5] fix: pass the same userSettings object to VisualizationPlugin props This avoids a double render issue which causes the Highcharts config generation to run twice. --- .../components/Visualization/Visualization.js | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/app/src/components/Visualization/Visualization.js b/packages/app/src/components/Visualization/Visualization.js index bd8a31a10e..98bf741f15 100644 --- a/packages/app/src/components/Visualization/Visualization.js +++ b/packages/app/src/components/Visualization/Visualization.js @@ -155,14 +155,20 @@ export class Visualization extends Component { } render() { - const { visualization, visFilters, error } = this.props + const { + visualization, + visFilters, + userSettings, + error, + isLoading, + } = this.props const { renderId } = this.state return !visualization || error ? ( ) : ( - {this.props.isLoading ? ( + {isLoading ? (
@@ -177,9 +183,7 @@ export class Visualization extends Component { onError={this.onError} onDrill={this.onDrill} style={styles.chartCanvas} - userSettings={{ - displayProperty: this.props.displayProperty, - }} + userSettings={userSettings} /> ) @@ -189,7 +193,6 @@ export class Visualization extends Component { Visualization.propTypes = { addMetadata: PropTypes.func, addParentGraphMap: PropTypes.func, - displayProperty: PropTypes.string, error: PropTypes.object, isLoading: PropTypes.bool, rightSidebarOpen: PropTypes.bool, @@ -197,6 +200,7 @@ Visualization.propTypes = { setCurrent: PropTypes.func, setLoadError: PropTypes.func, setUiItems: PropTypes.func, + userSettings: PropTypes.object, visFilters: PropTypes.object, visualization: PropTypes.object, onLoadingComplete: PropTypes.func, @@ -216,13 +220,20 @@ export const visFiltersSelector = createSelector( : {} ) +export const userSettingsSelector = createSelector( + [sGetSettingsDisplayNameProperty], + displayProperty => ({ + displayProperty, + }) +) + const mapStateToProps = state => ({ visualization: visualizationSelector(state), visFilters: visFiltersSelector(state), rightSidebarOpen: sGetUiRightSidebarOpen(state), error: sGetLoadError(state), isLoading: sGetIsPluginLoading(state), - displayProperty: sGetSettingsDisplayNameProperty(state), + userSettings: userSettingsSelector(state), }) const mapDispatchToProps = dispatch => ({ From d6c542f9eb347c1b23f129edbaa429ceb94b12fb Mon Sep 17 00:00:00 2001 From: Edoardo Sabadelli Date: Wed, 17 Mar 2021 16:15:00 +0100 Subject: [PATCH 3/5] fix: pass the same userSettings object to VisualizationPlugin props This avoids a double render issue which causes the Highcharts config generation to run twice. --- .../components/Visualization/Visualization.js | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/app/src/components/Visualization/Visualization.js b/packages/app/src/components/Visualization/Visualization.js index bd8a31a10e..98bf741f15 100644 --- a/packages/app/src/components/Visualization/Visualization.js +++ b/packages/app/src/components/Visualization/Visualization.js @@ -155,14 +155,20 @@ export class Visualization extends Component { } render() { - const { visualization, visFilters, error } = this.props + const { + visualization, + visFilters, + userSettings, + error, + isLoading, + } = this.props const { renderId } = this.state return !visualization || error ? ( ) : ( - {this.props.isLoading ? ( + {isLoading ? (
@@ -177,9 +183,7 @@ export class Visualization extends Component { onError={this.onError} onDrill={this.onDrill} style={styles.chartCanvas} - userSettings={{ - displayProperty: this.props.displayProperty, - }} + userSettings={userSettings} /> ) @@ -189,7 +193,6 @@ export class Visualization extends Component { Visualization.propTypes = { addMetadata: PropTypes.func, addParentGraphMap: PropTypes.func, - displayProperty: PropTypes.string, error: PropTypes.object, isLoading: PropTypes.bool, rightSidebarOpen: PropTypes.bool, @@ -197,6 +200,7 @@ Visualization.propTypes = { setCurrent: PropTypes.func, setLoadError: PropTypes.func, setUiItems: PropTypes.func, + userSettings: PropTypes.object, visFilters: PropTypes.object, visualization: PropTypes.object, onLoadingComplete: PropTypes.func, @@ -216,13 +220,20 @@ export const visFiltersSelector = createSelector( : {} ) +export const userSettingsSelector = createSelector( + [sGetSettingsDisplayNameProperty], + displayProperty => ({ + displayProperty, + }) +) + const mapStateToProps = state => ({ visualization: visualizationSelector(state), visFilters: visFiltersSelector(state), rightSidebarOpen: sGetUiRightSidebarOpen(state), error: sGetLoadError(state), isLoading: sGetIsPluginLoading(state), - displayProperty: sGetSettingsDisplayNameProperty(state), + userSettings: userSettingsSelector(state), }) const mapDispatchToProps = dispatch => ({ From 0cef7b4c69a34fea14745da6552b1cc2cd089234 Mon Sep 17 00:00:00 2001 From: Edoardo Sabadelli Date: Thu, 18 Mar 2021 14:51:45 +0100 Subject: [PATCH 4/5] refactor: list all used props in the deconstructor --- packages/app/src/components/Visualization/Visualization.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/app/src/components/Visualization/Visualization.js b/packages/app/src/components/Visualization/Visualization.js index 98bf741f15..7f1f285012 100644 --- a/packages/app/src/components/Visualization/Visualization.js +++ b/packages/app/src/components/Visualization/Visualization.js @@ -161,6 +161,7 @@ export class Visualization extends Component { userSettings, error, isLoading, + onLoadingComplete, } = this.props const { renderId } = this.state @@ -178,7 +179,7 @@ export class Visualization extends Component { visualization={visualization} filters={visFilters} onChartGenerated={this.onChartGenerated} - onLoadingComplete={this.props.onLoadingComplete} + onLoadingComplete={onLoadingComplete} onResponsesReceived={this.onResponsesReceived} onError={this.onError} onDrill={this.onDrill} From 9e4bc0b8f5748c3b1776991a96def21869f3d146 Mon Sep 17 00:00:00 2001 From: Edoardo Sabadelli Date: Fri, 19 Mar 2021 11:24:10 +0100 Subject: [PATCH 5/5] fix: init id as null to avoid rerender when not passed This happened in dashboards where id is not passed to ChartPlugin because the re-render on resize with no animation is only needed in DV. --- packages/plugin/src/ChartPlugin.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/plugin/src/ChartPlugin.js b/packages/plugin/src/ChartPlugin.js index f8c42dd34f..9021853af4 100644 --- a/packages/plugin/src/ChartPlugin.js +++ b/packages/plugin/src/ChartPlugin.js @@ -71,6 +71,7 @@ ChartPlugin.defaultProps = { filters: {}, style: {}, animation: 200, + id: null, onChartGenerated: Function.prototype, }