Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: use hooks to exert finer control on setting dashboard [DHIS2-9508] #1067

Merged
merged 10 commits into from
Sep 15, 2020
Merged

fix: use hooks to exert finer control on setting dashboard [DHIS2-9508] #1067

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
060e783
fix: use hooks to exert finer control on setting dashboard
jenniferarnesen Sep 14, 2020
d9bdc13
fix: removeEventListener
jenniferarnesen Sep 15, 2020
d85c69d
Merge branch 'master' into fix/reduce-redux-repetition
janhenrikoverland Sep 15, 2020
81abcfb
fix: fix the redux mutation
jenniferarnesen Sep 15, 2020
2005ef6
Merge branch 'fix/reduce-redux-repetition' of github.com:dhis2/dashbo…
jenniferarnesen Sep 15, 2020
d8ab02d
fix: use classnames
jenniferarnesen Sep 15, 2020
824bb99
fix: issue with oipp print title page size
jenniferarnesen Sep 15, 2020
2070368
fix: remove unused constant
jenniferarnesen Sep 15, 2020
d80375f
fix: move constants to same file
jenniferarnesen Sep 15, 2020
38b5989
fix: proptype, better var name and ensure prop defined
jenniferarnesen Sep 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 70 additions & 69 deletions src/components/Dashboard/Dashboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { Component } from 'react'
import React, { useEffect } from 'react'
import { connect } from 'react-redux'
import PropTypes from 'prop-types'
import isEmpty from 'lodash/isEmpty'
Expand Down Expand Up @@ -34,6 +34,15 @@ import {
isPrintMode,
} from './dashboardModes'

const setHeaderbarVisibility = mode => {
const header = document.getElementsByTagName('header')[0]
if (isPrintMode(mode)) {
header.classList.add('hidden')
} else {
header.classList.remove('hidden')
}
}

jenniferarnesen marked this conversation as resolved.
Show resolved Hide resolved
const dashboardMap = {
[VIEW]: <ViewDashboard />,
[EDIT]: <EditDashboard />,
Expand All @@ -42,101 +51,93 @@ const dashboardMap = {
[PRINT_LAYOUT]: <PrintLayoutDashboard />,
}

export class Dashboard extends Component {
setDashboard = () => {
if (this.props.dashboardsLoaded) {
const id = this.props.match.params.dashboardId || null || null

this.props.selectDashboard(id)

this.setHeaderbarVisibility()
}
}

setHeaderbarVisibility = () => {
const header = document.getElementsByTagName('header')[0]
if (isPrintMode(this.props.mode)) {
header.classList.add('hidden')
} else {
header.classList.remove('hidden')
export const Dashboard = ({
id,
mode,
dashboardsLoaded,
dashboardsIsEmpty,
routeId,
selectDashboard,
setWindowHeight,
}) => {
useEffect(() => {
setHeaderbarVisibility(mode)
}, [mode])

useEffect(() => {
if (dashboardsLoaded && !dashboardsIsEmpty) {
selectDashboard(routeId)
}
}
}, [dashboardsLoaded, dashboardsIsEmpty, routeId, mode])
jenniferarnesen marked this conversation as resolved.
Show resolved Hide resolved

componentDidMount() {
useEffect(() => {
// TODO does this need to be cleared?
window.onresize = debounce(
() => this.props.setWindowHeight(window.innerHeight),
() => setWindowHeight(window.innerHeight),
jenniferarnesen marked this conversation as resolved.
Show resolved Hide resolved
300
)
this.setDashboard()
}, [])

if (!dashboardsLoaded || id === null) {
return (
<Layer translucent>
<CenteredContent>
<CircularLoader />
</CenteredContent>
</Layer>
)
}

componentDidUpdate() {
this.setDashboard()
if (mode === NEW) {
return dashboardMap[mode]
}

render() {
const { id, mode, dashboardsLoaded, dashboardsIsEmpty } = this.props

if (!dashboardsLoaded || id === null) {
return (
<Layer translucent>
<CenteredContent>
<CircularLoader />
</CenteredContent>
</Layer>
)
}

if (mode === NEW) {
return dashboardMap[mode]
}

if (dashboardsIsEmpty) {
return (
<>
<DashboardsBar />
<DashboardVerticalOffset />
<NoContentMessage
text={i18n.t(
'No dashboards found. Use the + button to create a new dashboard.'
)}
/>
</>
)
}

if (id === NON_EXISTING_DASHBOARD_ID) {
return (
<>
<DashboardsBar />
<DashboardVerticalOffset />
<NoContentMessage
text={i18n.t('Requested dashboard not found')}
/>
</>
)
}
if (dashboardsIsEmpty) {
return (
<>
<DashboardsBar />
<DashboardVerticalOffset />
<NoContentMessage
text={i18n.t(
'No dashboards found. Use the + button to create a new dashboard.'
)}
/>
</>
)
}

return dashboardMap[mode]
if (id === NON_EXISTING_DASHBOARD_ID) {
return (
<>
<DashboardsBar />
<DashboardVerticalOffset />
<NoContentMessage
text={i18n.t('Requested dashboard not found')}
/>
</>
)
}

return dashboardMap[mode]
}

Dashboard.propTypes = {
dashboardsIsEmpty: PropTypes.bool,
dashboardsLoaded: PropTypes.bool,
id: PropTypes.string,
match: PropTypes.object,
jenniferarnesen marked this conversation as resolved.
Show resolved Hide resolved
mode: PropTypes.string,
routeId: PropTypes.string,
selectDashboard: PropTypes.func,
setWindowHeight: PropTypes.func,
}

const mapStateToProps = state => {
const mapStateToProps = (state, ownProps) => {
const dashboards = sGetAllDashboards(state)
return {
dashboardsIsEmpty: isEmpty(dashboards),
dashboardsLoaded: !sDashboardsIsFetching(state),
id: sGetSelectedId(state),
routeId: ownProps.match.params.dashboardId || null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break with a reference error if match or params is undefined - we should either define an explicit shape requirement in propTypes or use optional chaining (ownProps.match?.params?.dashboardId) here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

match could also be just a regular prop, and use const routeId = match?.params?.dashboardId || null could be at the top of the component body

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add that. I did move match.params.dashboardId from componentDidMount. It had been like that since the original dashboard code, so I figured it will always be defined. But best to be sure.

}
}

Expand Down