fix: avoid visualization plugin double render (#1665)

* refactor: remove unused action * fix: pass the same userSettings object to VisualizationPlugin props This avoids a double render issue which causes the Highcharts config generation to run twice. * refactor: list all used props in the deconstructor * 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. Co-authored-by: Martin Ohlson <martin@moid.se>
dhis2 · Mar 31, 2021 · 88bd9db · 88bd9db
1 parent 43c4842
commit 88bd9db
Show file tree

Hide file tree

Showing 5 changed files with 23 additions and 39 deletions.
diff --git 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/components/Visualization/Visualization.js b/packages/app/src/components/Visualization/Visualization.js
@@ -155,14 +155,21 @@ export class Visualization extends Component {
     }
 
     render() {
-        const { visualization, visFilters, error } = this.props
+        const {
+            visualization,
+            visFilters,
+            userSettings,
+            error,
+            isLoading,
+            onLoadingComplete,
+        } = this.props
         const { renderId } = this.state
 
         return !visualization || error ? (
             <StartScreen />
         ) : (
             <Fragment>
-                {this.props.isLoading ? (
+                {isLoading ? (
                     <div style={styles.loadingCover}>
                         <LoadingMask />
                     </div>
@@ -172,14 +179,12 @@ 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}
                     style={styles.chartCanvas}
-                    userSettings={{
-                        displayProperty: this.props.displayProperty,
-                    }}
+                    userSettings={userSettings}
                 />
             </Fragment>
         )
@@ -189,14 +194,14 @@ 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,
     setChart: PropTypes.func,
     setCurrent: PropTypes.func,
     setLoadError: PropTypes.func,
     setUiItems: PropTypes.func,
+    userSettings: PropTypes.object,
     visFilters: PropTypes.object,
     visualization: PropTypes.object,
     onLoadingComplete: PropTypes.func,
@@ -216,13 +221,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 => ({

diff --git 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
@@ -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,

diff --git 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,
 }