-
Notifications
You must be signed in to change notification settings - Fork 22
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
fix: [DHIS2-16801] events scheduled for today's date not showing today #3856
fix: [DHIS2-16801] events scheduled for today's date not showing today #3856
Conversation
return { status: statusTypes.SCHEDULE, options: undefined }; | ||
} | ||
|
||
if (daysUntilDueDate === 0) { | ||
return { status: statusTypes.SCHEDULE, options: 'today' }; |
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.
Is this the string that get's displayed in the badge? If so, do you think it makes sense to add translations?
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.
@eirikhaugstulen No, it isn't the string that ended up being displayed, it gets passed to this function here and get translated.
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 string is passed as a dynamic value into the translation. As far as I understand from the i18next interpolation docs, a dynamic value is not translated. When I change the language to French, this is what is currently displayed:
By adding translations option: i18n.t('Today')
as Eirik suggested, the text is translated correctly:
Could you please recheck @alaa-yahia? Let me know if you need any help or have doubts. Thank you!
PS: For these examples, I deliberately chose 'Today' in uppercase because the string already exists translated in the language files, but you can also use lowercase and Transfix will add the translations in a few weeks.
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.
Ah whoops! I noticed dueDateFromNow was being translated, so I figured the same would happen with today's date. Didn't realize moment.js has a localization.
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.
It looks great now! Good job @alaa-yahia 🎉
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.
Well done @alaa-yahia! 🎉
0db8ee0
to
c06023e
Compare
🚀 Deployed on https://deploy-preview-3856.capture.netlify.dhis2.org |
…scheduled-for-today-date-should-show-today
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.
Tested successfully on 2.42,2.41.3,2.40.7,2.37.8 versions
## [101.16.7](v101.16.6...v101.16.7) (2024-11-25) ### Bug Fixes * [DHIS2-16801] events scheduled for today's date not showing today ([#3856](#3856)) ([d63e124](d63e124))
🎉 This PR is included in version 101.16.7 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Implements DHIS2-16801