From 99453592a8ab84af12f5f1aa86d5389a1a5408b0 Mon Sep 17 00:00:00 2001 From: Edoardo Sabadelli Date: Mon, 10 Feb 2020 13:56:20 +0100 Subject: [PATCH 1/2] fix: remove custom title when Auto is selected (DHIS2-8252) Also fixed the subtitle option to work in the same way as the title. Before when no custom subtitle was specified, the Custom subtitle was selected with an empty value. Now Auto means not hidden subtitle but also not custom subtitle. The subtitle is actually auto generated in some situations (depending on vis type, presence of title...) --- .../Options/HideSubtitle.js | 24 +++++++++++++++---- .../VisualizationOptions/Options/HideTitle.js | 13 ++++++---- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/packages/app/src/components/VisualizationOptions/Options/HideSubtitle.js b/packages/app/src/components/VisualizationOptions/Options/HideSubtitle.js index e597422d20..d81b99fb75 100644 --- a/packages/app/src/components/VisualizationOptions/Options/HideSubtitle.js +++ b/packages/app/src/components/VisualizationOptions/Options/HideSubtitle.js @@ -1,6 +1,7 @@ import React, { Component } from 'react' import PropTypes from 'prop-types' import { connect } from 'react-redux' +import { createSelector } from 'reselect' import i18n from '@dhis2/d2-i18n' import { Label, Radio, RadioGroup } from '@dhis2/ui-core' @@ -20,14 +21,18 @@ class HideSubtitle extends Component { constructor(props) { super(props) - this.defaultState = { value: 'NONE' } + this.defaultState = { value: 'AUTO' } this.state = props.value ? { value: props.value } : this.defaultState } onChange = ({ value }) => { this.setState({ value }) - this.props.onChange(value === 'NONE') + this.props.onChange({ + hideSubtitle: value === 'NONE', + subtitle: + value === 'AUTO' ? undefined : this.props.subtitle || undefined, + }) } render() { @@ -48,6 +53,7 @@ class HideSubtitle extends Component { dense > {[ + { id: 'AUTO', label: i18n.t('Auto generated') }, { id: 'NONE', label: i18n.t('None') }, { id: 'CUSTOM', label: i18n.t('Custom') }, ].map(({ id, label }) => ( @@ -65,18 +71,28 @@ class HideSubtitle extends Component { } HideSubtitle.propTypes = { + subtitle: PropTypes.string, value: PropTypes.string, visualizationType: PropTypes.string, onChange: PropTypes.func, } +const hideSubtitleSelector = createSelector([sGetUiOptions], uiOptions => + uiOptions.hideSubtitle + ? 'NONE' + : uiOptions.subtitle === undefined + ? 'AUTO' + : 'CUSTOM' +) + const mapStateToProps = state => ({ visualizationType: sGetUiType(state), - value: sGetUiOptions(state).hideSubtitle ? 'NONE' : 'CUSTOM', + value: hideSubtitleSelector(state), + subtitle: sGetUiOptions(state).subtitle, }) const mapDispatchToProps = dispatch => ({ - onChange: enabled => dispatch(acSetUiOptions({ hideSubtitle: enabled })), + onChange: value => dispatch(acSetUiOptions(value)), }) export default connect(mapStateToProps, mapDispatchToProps)(HideSubtitle) diff --git a/packages/app/src/components/VisualizationOptions/Options/HideTitle.js b/packages/app/src/components/VisualizationOptions/Options/HideTitle.js index c15106776b..a3ded434e6 100644 --- a/packages/app/src/components/VisualizationOptions/Options/HideTitle.js +++ b/packages/app/src/components/VisualizationOptions/Options/HideTitle.js @@ -26,9 +26,12 @@ class HideTitle extends Component { this.state = props.value ? { value: props.value } : this.defaultState } - onChange = ({ value }) => { + onRadioGroupChange = ({ value }) => { this.setState({ value }) - this.props.onChange(value === 'NONE') + this.props.onChange({ + hideTitle: value === 'NONE', + title: value === 'AUTO' ? undefined : this.props.title || undefined, + }) } render() { @@ -44,7 +47,7 @@ class HideTitle extends Component { @@ -67,6 +70,7 @@ class HideTitle extends Component { } HideTitle.propTypes = { + title: PropTypes.string, value: PropTypes.string, visualizationType: PropTypes.string, onChange: PropTypes.func, @@ -83,10 +87,11 @@ const hideTitleSelector = createSelector([sGetUiOptions], uiOptions => const mapStateToProps = state => ({ visualizationType: sGetUiType(state), value: hideTitleSelector(state), + title: sGetUiOptions(state).title, }) const mapDispatchToProps = dispatch => ({ - onChange: enabled => dispatch(acSetUiOptions({ hideTitle: enabled })), + onChange: value => dispatch(acSetUiOptions(value)), }) export default connect(mapStateToProps, mapDispatchToProps)(HideTitle) From 71d17dba78197112d09c230e2929b3052c8c44c4 Mon Sep 17 00:00:00 2001 From: Edoardo Sabadelli Date: Mon, 10 Feb 2020 15:58:37 +0100 Subject: [PATCH 2/2] chore: cleanup, implemented Martin's suggestions --- .../Options/HideSubtitle.js | 37 ++++++++++++------- .../VisualizationOptions/Options/HideTitle.js | 36 +++++++++++------- 2 files changed, 46 insertions(+), 27 deletions(-) diff --git a/packages/app/src/components/VisualizationOptions/Options/HideSubtitle.js b/packages/app/src/components/VisualizationOptions/Options/HideSubtitle.js index d81b99fb75..476e992322 100644 --- a/packages/app/src/components/VisualizationOptions/Options/HideSubtitle.js +++ b/packages/app/src/components/VisualizationOptions/Options/HideSubtitle.js @@ -17,22 +17,27 @@ import { tabSectionOptionToggleable, } from '../styles/VisualizationOptions.style.js' +const HIDE_SUBTITLE_AUTO = 'AUTO' +const HIDE_SUBTITLE_NONE = 'NONE' +const HIDE_SUBTITLE_CUSTOM = 'CUSTOM' + class HideSubtitle extends Component { constructor(props) { super(props) - this.defaultState = { value: 'AUTO' } + this.defaultState = { value: HIDE_SUBTITLE_AUTO } this.state = props.value ? { value: props.value } : this.defaultState } onChange = ({ value }) => { this.setState({ value }) - this.props.onChange({ - hideSubtitle: value === 'NONE', - subtitle: - value === 'AUTO' ? undefined : this.props.subtitle || undefined, - }) + this.props.onChange( + value === HIDE_SUBTITLE_NONE, + value === HIDE_SUBTITLE_AUTO + ? undefined + : this.props.subtitle || undefined + ) } render() { @@ -53,14 +58,17 @@ class HideSubtitle extends Component { dense > {[ - { id: 'AUTO', label: i18n.t('Auto generated') }, - { id: 'NONE', label: i18n.t('None') }, - { id: 'CUSTOM', label: i18n.t('Custom') }, + { + id: HIDE_SUBTITLE_AUTO, + label: i18n.t('Auto generated'), + }, + { id: HIDE_SUBTITLE_NONE, label: i18n.t('None') }, + { id: HIDE_SUBTITLE_CUSTOM, label: i18n.t('Custom') }, ].map(({ id, label }) => ( ))} - {value === 'CUSTOM' ? ( + {value === HIDE_SUBTITLE_CUSTOM ? (
@@ -79,10 +87,10 @@ HideSubtitle.propTypes = { const hideSubtitleSelector = createSelector([sGetUiOptions], uiOptions => uiOptions.hideSubtitle - ? 'NONE' + ? HIDE_SUBTITLE_NONE : uiOptions.subtitle === undefined - ? 'AUTO' - : 'CUSTOM' + ? HIDE_SUBTITLE_AUTO + : HIDE_SUBTITLE_CUSTOM ) const mapStateToProps = state => ({ @@ -92,7 +100,8 @@ const mapStateToProps = state => ({ }) const mapDispatchToProps = dispatch => ({ - onChange: value => dispatch(acSetUiOptions(value)), + onChange: (hideSubtitle, subtitle) => + dispatch(acSetUiOptions({ hideSubtitle, subtitle })), }) export default connect(mapStateToProps, mapDispatchToProps)(HideSubtitle) diff --git a/packages/app/src/components/VisualizationOptions/Options/HideTitle.js b/packages/app/src/components/VisualizationOptions/Options/HideTitle.js index a3ded434e6..0860b87145 100644 --- a/packages/app/src/components/VisualizationOptions/Options/HideTitle.js +++ b/packages/app/src/components/VisualizationOptions/Options/HideTitle.js @@ -17,21 +17,27 @@ import { tabSectionOptionToggleable, } from '../styles/VisualizationOptions.style.js' +const HIDE_TITLE_AUTO = 'AUTO' +const HIDE_TITLE_NONE = 'NONE' +const HIDE_TITLE_CUSTOM = 'CUSTOM' + class HideTitle extends Component { constructor(props) { super(props) - this.defaultState = { value: 'AUTO' } + this.defaultState = { value: HIDE_TITLE_AUTO } this.state = props.value ? { value: props.value } : this.defaultState } onRadioGroupChange = ({ value }) => { this.setState({ value }) - this.props.onChange({ - hideTitle: value === 'NONE', - title: value === 'AUTO' ? undefined : this.props.title || undefined, - }) + this.props.onChange( + value === HIDE_TITLE_NONE, + value === HIDE_TITLE_AUTO + ? undefined + : this.props.title || undefined + ) } render() { @@ -52,14 +58,17 @@ class HideTitle extends Component { dense > {[ - { id: 'AUTO', label: i18n.t('Auto generated') }, - { id: 'NONE', label: i18n.t('None') }, - { id: 'CUSTOM', label: i18n.t('Custom') }, + { + id: HIDE_TITLE_AUTO, + label: i18n.t('Auto generated'), + }, + { id: HIDE_TITLE_NONE, label: i18n.t('None') }, + { id: HIDE_TITLE_CUSTOM, label: i18n.t('Custom') }, ].map(({ id, label }) => ( ))} - {value === 'CUSTOM' ? ( + {value === HIDE_TITLE_CUSTOM ? (
</div> @@ -78,10 +87,10 @@ HideTitle.propTypes = { const hideTitleSelector = createSelector([sGetUiOptions], uiOptions => uiOptions.hideTitle - ? 'NONE' + ? HIDE_TITLE_NONE : uiOptions.title === undefined - ? 'AUTO' - : 'CUSTOM' + ? HIDE_TITLE_AUTO + : HIDE_TITLE_CUSTOM ) const mapStateToProps = state => ({ @@ -91,7 +100,8 @@ const mapStateToProps = state => ({ }) const mapDispatchToProps = dispatch => ({ - onChange: value => dispatch(acSetUiOptions(value)), + onChange: (hideTitle, title) => + dispatch(acSetUiOptions({ hideTitle, title })), }) export default connect(mapStateToProps, mapDispatchToProps)(HideTitle)