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.

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

Conversation

alaa-yahia
Copy link
Member

@alaa-yahia alaa-yahia commented Oct 23, 2024

Implements DHIS2-16801


image

@alaa-yahia alaa-yahia requested a review from a team as a code owner October 23, 2024 10:26
return { status: statusTypes.SCHEDULE, options: undefined };
}

if (daysUntilDueDate === 0) {
return { status: statusTypes.SCHEDULE, options: 'today' };
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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:

  1. Screenshot 2024-10-23 at 17 46 06

By adding translations option: i18n.t('Today') as Eirik suggested, the text is translated correctly:

  1. Screenshot 2024-10-23 at 17 19 49

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.

Copy link
Member Author

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.

Copy link
Contributor

@simonadomnisoru simonadomnisoru left a 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 🎉

Copy link
Contributor

@eirikhaugstulen eirikhaugstulen left a 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! 🎉

@alaa-yahia alaa-yahia force-pushed the DHIS2-16801-events-scheduled-for-today-date-should-show-today branch from 0db8ee0 to c06023e Compare November 5, 2024 08:16
Copy link

github-actions bot commented Nov 5, 2024

Copy link

@geethaalwan geethaalwan left a 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

@alaa-yahia alaa-yahia merged commit d63e124 into master Nov 25, 2024
55 of 60 checks passed
@alaa-yahia alaa-yahia deleted the DHIS2-16801-events-scheduled-for-today-date-should-show-today branch November 25, 2024 10:49
dhis2-bot added a commit that referenced this pull request Nov 25, 2024
## [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))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 101.16.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants