-
-
Notifications
You must be signed in to change notification settings - Fork 326
feat(#775): support function-based target subtitles #10507
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
base: 10140_previous-month-targets
Are you sure you want to change the base?
feat(#775): support function-based target subtitles #10507
Conversation
| {{ target.subtitle_translation_key | translate }} | ||
| </p> | ||
| <p> | ||
| {{ |
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.
@Edwin-Rugoogamu this is not what we discussed, adding two subtitles.
We discussed making target.subtitle_translation_key be a function.
So this would be something like:
target.subtitle_translation_key(reportingPeriodFilter) | translate
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.
@dianabarsan Thankyou for the feedback. let me adjust it .
| if (!subtitleKey) { | ||
| return ''; | ||
| } | ||
| return reportingPeriod === ReportingPeriod.CURRENT ? 'this_month' : 'all_time'; |
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.
This still doesn't cover what we need to do here. The situation now is that you're not even using the subtitleKey that is configured.
What needs to change here is that subtitleKey will be a function!
So you'll need to do something like:
return typeof subtitleKey === 'function' ? subtitleKey(reportingPeriod) : subtitleKey;
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.
@dianabarsan Thankyou for the feedback. We had a meeting with Josh and Team . This elaborated more on what we need to achieve. So it made it clear for me and erased the confusion i was having. So am adjusting and will send a PR for review.
Description
Code review checklist
can_view_old_navigationpermission to see the old design. Test it has appropriate design for RTL languages.License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.