-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
RN-613: Export Selected Dashboard Only #4130
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for some clean up and making the changes to export one dashboard only! It makes everything easier.
Just one request on whether we could remove passing in previewDashboards
, previewDashboardValue
.
const exportableDashboards = | ||
viewType === DASHBOARD_EXPORT_PREVIEW | ||
? previewDashboards | ||
: getExportableDashboardsForDownloadView(exportViewProps.exportOptions.dashboard); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get dashboardValue
directly from URL, then no need to pass in previewDashboards
and previewDashboardValue
? If so it helps code reading.
previewDashboards
should be the same as exportableDashboards
? It can be retrieved by getExportableDashboardsForDownloadView()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, great idea. Have reduced the props now.
@@ -38,8 +38,7 @@ export const DashboardView = React.memo(({ isOpen, setIsOpen }) => { | |||
const [selectedYear, setSelectedYear] = useUrlSearchParam('year', DEFAULT_DATA_YEAR); | |||
|
|||
const { dropdownOptions, selectedOption } = useDashboardDropdownOptions(); | |||
const profileDropDownOptions = dropdownOptions.filter(({ exportToPDF }) => exportToPDF); | |||
const { exportableDashboards, totalPage } = getExportableDashboards(profileDropDownOptions); | |||
const { exportableDashboards, totalPage } = getExportableDashboards([selectedOption]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clean up 🙏
Can we have a small change on getExportableDashboards([selectedOption]) -> getExportableDashboards(selectedOption)
?
We do not need to export from multiple dashboards anymore, it would be worth it to clean up here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@billli0 I've refactored this code.
@@ -10,7 +10,7 @@ import { stringifyQuery } from '@tupaia/utils'; | |||
import { post } from '../../api'; | |||
|
|||
export const useDashboardItemsExportToPDF = options => { | |||
const { locale, entityCode, ...restOfoptions } = options; | |||
const { locale, entityCode, ...restOfOptions } = options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
const dropdownOptions = [ | ||
{ | ||
value: 'profile', | ||
exportToPDF: true, | ||
}, | ||
{ | ||
value: 'indicators', | ||
exportToPDF: true, | ||
}, | ||
{ | ||
value: 'ESSDP_Plan', | ||
}, | ||
...SUB_DASHBOARD_OPTIONS.map(dashboard => ({ | ||
value: dashboard.code, | ||
|
||
exportToPDF: dashboard.exportToPDF, | ||
})), | ||
]; | ||
|
||
const selectedOption = | ||
selectedDashboard && dropdownOptions.find(option => option.value === selectedDashboard); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can directly get selectedOption
from useDashboardDropdownOptions()
.
@@ -90,15 +93,18 @@ const getChildren = ({ | |||
export const ExportView = ({ viewProps, viewType, className }) => { | |||
const { getExtraExportViewProps, PageContainer, PageContent } = EXPORT_VIEWS[viewType]; | |||
const exportViewProps = { ...viewProps, ...getExtraExportViewProps() }; | |||
|
|||
const { selectedDashboard } = exportViewProps; | |||
const { dropdownOptions } = useDashboardDropdownOptions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { dropdownOptions } = useDashboardDropdownOptions(); | |
const { selectedOption } = useDashboardDropdownOptions(); |
We can get the selectedOption
here, no need to find the selected dashboard below.
includeDrillDowns: false, | ||
}); | ||
|
||
if (dropdownOption.exportToPDF === undefined || !dropdownOption.exportToPDF) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (dropdownOption.exportToPDF === undefined || !dropdownOption.exportToPDF) { | |
if (!dropdownOption.exportToPDF) { |
viewType === DASHBOARD_EXPORT_PREVIEW | ||
? selectedDashboard === 'profile' && index === 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
viewType === DASHBOARD_EXPORT_PREVIEW | |
? selectedDashboard === 'profile' && index === 0 | |
selectedDashboard === 'profile' && index === 0 |
We should show the entity profile in preview and export view?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chris-pollard for doing some clean up. I have a few more requests on refactoring the code 🙏, but overall looks good!
if (value === 'indicators') { | ||
return () => false; | ||
} | ||
const filter = | ||
value === 'profile' | ||
? ({ dashboardCode }) => | ||
!Object.values(SUB_DASHBOARD_OPTIONS).some(({ code }) => | ||
dashboardCode.startsWith(`LESMIS_${code}`), | ||
) | ||
: ({ dashboardCode }) => dashboardCode.startsWith(`LESMIS_${value}`); | ||
return filter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a switch here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for refactoring SUB_DASHBOARD_OPTIONS
along the PR, super helpful. Just a minor request, can approve in advance.
Issue RN-613:
Changes: