-
Notifications
You must be signed in to change notification settings - Fork 15
feat: enabling and disabling of data output period types #1452
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
Conversation
|
🚀 Deployed on https://pr-1452--dhis2-settings.netlify.app |
103f275 to
2076c14
Compare
# [100.11.0](v100.10.0...v100.11.0) (2025-11-18) ### Features * add orgUnitCentroidsInEventsAnalytics to analytics settings ([cb81cde](cb81cde)) * add orgUnitCentroidsInEventsAnalytics to analytics settings ([e5c0ecf](e5c0ecf))
- Commented out the keyHideDailyPeriods, keyHideWeeklyPeriods, keyHideBiWeeklyPeriods, keyHideMonthlyPeriods, and keyHideBiMonthlyPeriods settings in settingsCategories.js and settingsKeyMapping.js. - Introduced the PeriodTypes component in settingsFields.component.jsx to handle period types settings.
2076c14 to
81e98d6
Compare
tomzemp
left a comment
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 good!
This needs to be feature toggled for v43.
For implementation, I think it might be better to keep the current allowed period types in state, and then update that state when the post succeeds (if post fails, do not update). That will save you from performing a refetch.
I also think that it might be better to just use the translations for the periods provided by the backend (e.g. use displayName from api/periodTypes). The labels are not as nice looking as the ones suggested by Joe (e.g. the backend has Weekly Wednesday as the display name, not Weekly (starting Wednesday), but the logic will be simpler and we'll pick up translations more reliably. (That's also partly a question for @cooper-joe). If you want to go with the nicer labels, I've noted some places where I saw the logic would need to be updated to get translations to flow through.
| } | ||
|
|
||
| const monthMap = { | ||
| April: 'April', |
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.
these should be translated
|
|
||
| if (name.startsWith('Weekly')) { | ||
| const day = name.replace('Weekly', '') | ||
| return day |
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.
the day won't be translated here, so you'd need to also create a dayMap and add translations for the weekdays
| setUpdating(true) | ||
| try { | ||
| const d2 = await getD2() | ||
| const api = d2.Api.getApi() |
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 useDataMutation ? or is that not working for some reason. I think it would be better not to fetch with app-runtime tooling and post with d2 (also would prefer to not use d2 in any new code that is not relying heavily on existing settings app code)
| setting: 'keyAnalysisDigitGroupSeparator', | ||
| }, | ||
| { | ||
| setting: 'keyHideDailyPeriods', |
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.
these need to remain (at least for v42-, and TBD for v43+)
|
tomzemp
left a comment
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 good!
|
🎉 This PR is included in version 100.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |



Screen.Recording.2025-12-17.at.10.52.38.mov