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

feat: date time formats from locales #4029

Merged
merged 21 commits into from
Oct 23, 2024
Merged

Conversation

YUCLing
Copy link
Contributor

@YUCLing YUCLing commented Sep 27, 2024

Fixes flarum-lang/chinese-simplified#23 and flarum-lang/chinese-simplified#27 (in another way)

Changes proposed in this pull request:
This PR adds some time formats to dayjs, which makes the customized time formats used by Flarum localizable in locale object of dayjs locales.

This PR makes Flarum formats all date time using the formats from the language packs.

The PR also made some improvements to humanTime and liveHumanTimes's comments.

Reviewers should focus on:
The new formats' naming and whether is it a good implementation.

All language packs should contain the following formats after this is merged:

  • f and F: For humanTime
  • ff and FF: For scrubber

It's not necessary though, a fallback will be used if they are not provided.

Check if all date time are formatted using the format from the language pack.

Although I've tested on my local Flarum installation, it would be nice for you to test it again.

Screenshot

99f92af943c71dfd7c6a05685f23a12d
513c8b085de4959658de0f73765b49c6

QA
All places where humanTime is used and post stream's scrubber.

Modify date time formats in locale and see if it works.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@YUCLing YUCLing marked this pull request as ready for review September 27, 2024 18:45
@YUCLing YUCLing requested a review from a team as a code owner September 27, 2024 18:45
framework/core/js/src/common/index.ts Outdated Show resolved Hide resolved
framework/core/js/src/common/utils/dayjsPlugins.ts Outdated Show resolved Hide resolved
@ExerciseBook
Copy link

I have another question, because we are adding features in Flarum core, can we make this feature more configurable?

such as

> new Intl.DateTimeFormat('zh-TW-u-ca-roc', {  year: 'numeric',  month: 'long',  day: 'numeric', }).format(new Date(2025, 4, 14));
'民國114年5月14日'

so I prefer that we make a flarum extender thus language-pack or something else plugin can define a new format.

@YUCLing
Copy link
Contributor Author

YUCLing commented Sep 29, 2024

so I prefer that we make a flarum extender thus language-pack or something else plugin can define a new format.

This is possible with the version in my working tree. But I don't see the possible use cases.

@ExerciseBook
Copy link

@YUCLing
Copy link
Contributor Author

YUCLing commented Sep 29, 2024

I have another question, because we are adding features in Flarum core, can we make this feature more configurable?

such as

> new Intl.DateTimeFormat('zh-TW-u-ca-roc', {  year: 'numeric',  month: 'long',  day: 'numeric', }).format(new Date(2025, 4, 14));
'民國114年5月14日'

so I prefer that we make a flarum extender thus language-pack or something else plugin can define a new format.

I see what you mean, the problem is that this PR only solves the hardcoded format problem with Flarum and DayJS's Localized Format plugin. The feature you requested needs a big change to how Flarum and DayJS work.

@ExerciseBook
Copy link

make sense

@rob006
Copy link
Contributor

rob006 commented Sep 29, 2024

Note that there was some discussion about it in: https://discuss.flarum.org/d/22913-what-date-format-do-you-use

As for the problem, I would much more prefer format defined as part of forum translation, like:

ago = d.format(app.translator.trans('core.forum.date-format.humanTimeShort'));

It will be easier to understand and maintain for language pack maintainers, and you can quite easily adjust it also as a forum owner. And overall it seems to be simpler and less hacky solution than introducing some cryptic FF formats.

@YUCLing
Copy link
Contributor Author

YUCLing commented Sep 29, 2024

As for the problem, I would much more prefer format defined as part of forum translation

I think this is kinda messy since you have one set of formats in DayJS that cannot be easily edited by forum owners and another set of formats in Flarum locale system.

This is the reason I didn't implement it in this way at the beginning.

@rob006
Copy link
Contributor

rob006 commented Sep 29, 2024

I think this is kinda messy since you have one set of formats in DayJS that cannot be easily edited by forum owners and another set of formats in Flarum locale system.

I'm not sure what the problem is. Most language packs copied dayjs translations from the source without any changes (I actually had plans to automate it and fetch updates automatically from dayjs repo). This format can be quite cryptic and tricky to change, but there is no really other way to translate dayjs.

If your solution relies on editing dayjs translations, then it inherits all its problems: it is more problematic for language pack maintainers and much harder to override by forum owners. On the other hand, if we use app.translator.trans() to pick format, then adjustring in Weblate or using Linguist should be quite easy. And in the future Flarum could add option to override these formats in admin panel (it could just override specific translations from language pack).

@YUCLing
Copy link
Contributor Author

YUCLing commented Sep 29, 2024

On the other hand, if we use app.translator.trans() to pick format

Actually, my latest code on my working tree already removed the Localized Format plugin from DayJS, so it's possible to make it also use the formats available in the language pack instead of the one from DayJS locale.

In this way, the priority of the format can become like this:
Language pack (Flarum translator) > DayJS locale > Hardcoded default English format

So we can make all formats available in DayJS Localized Format plugin and the ones that Flarum uses editable in Flarum's translator.

Does this sound good to you?

@YUCLing YUCLing marked this pull request as draft September 29, 2024 15:15
@YUCLing YUCLing changed the title feat: flarum customized time formats WIP: feat: flarum customized time formats Sep 29, 2024
@YUCLing
Copy link
Contributor Author

YUCLing commented Sep 29, 2024

Some details of the changes made still need some discussion, the PR is now a draft.

chore: fetch format from translator
@YUCLing
Copy link
Contributor Author

YUCLing commented Sep 29, 2024

@rob006 I've pushed new changes, would you like to check if it looks good to you? I haven't tested it yet.

@rob006
Copy link
Contributor

rob006 commented Sep 29, 2024

Is there any reason why you implemented this in this way instead of passing app.translator.trans() result to format()?

@YUCLing
Copy link
Contributor Author

YUCLing commented Sep 30, 2024

Is there any reason why you implemented this in this way instead of passing app.translator.trans() result to format()?

I just did what DayJS did to implement the localized format and make it work with Flarum. Also made it extendable.

Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! much appreciated!

I think the FF ff formats with the dayjs plugin will only add to the confusion with date time formats and future maintenance.

I'm going to propose something a little more simple but more clear.

Let's add a format(format: string, datetime: Dayjs) to the Translator class in common.

this method would:

  1. check if the provided format exists in a custom item list (customFormats()) and use that.
  2. Fallback to app.translator.trans('core.lib.datetime_formats.'+format)
  3. Fallback to the actual format.

There are a lot of different formats used throughout the app, (+extensions) so this would give language packs more power over how those formats are changed.

The locale would be literally the format as a key, so:

core:
  lib:
    datetime_formats:
      "h A": "h A"
      "MMMM YYYY": "MMMM YYYY"
      "YYYY-MM-DD": "YYYY-MM-DD"
      "YYYY-MM-DD h:mm A": "YYYY-MM-DD h:mm A"
      "YYYY-MM-DD h:mm:ss A": "YYYY-MM-DD h:mm:ss A"
      "LL": "LL"
      "LLL": "LLL"
      "LLLL": "LLLL"

core would have one single example commented out. While language packs can fill more entries as needed.

The new method would be called like so:

app.translator.format('MMMM YYYY', datetime)

@YUCLing @rob006 what do you think? do we see any potential issues with that approach?

@rob006
Copy link
Contributor

rob006 commented Oct 1, 2024

@SychO9 I think it would be much more practical and intuitive to override/translate format used in specific situation/place, than just overwrite dayjs formats. For example in Polish language pack I would prefer to use LL instead of ll in humanTime(), but it does not mean that I want to overwrite ll everywhere.
Also, from translator perspective, it is much easier to understand key like core.lib.datetime_formats.humanTimeFull than core.lib.datetime_formats.LL. And spaces and special characters in translations keys could be a can of worms...

@YUCLing Sorry, it looks overcomplicated to me and I don't see a practical reason to implement this in that way.

@SychO9
Copy link
Member

SychO9 commented Oct 2, 2024

@rob006 if you want an extension like that, that's up to you. Not every forum needs more than 1 language, so for many it's a valid use case.

Point is, this is for custom behavior. Up to the developer/webmaster to determine what they want, up to the dev to make it happen. We just enable proper ways of extending behavior/components. Rather than leave things to hacking up the code. This enables us to make changes to the core code without worrying constantly about breaking backwards compatibility.

@YUCLing YUCLing changed the title feat: flarum customized time formats feat: date time formats from locales Oct 3, 2024
@rob006
Copy link
Contributor

rob006 commented Oct 3, 2024

@SychO9 My point is: if we don't know exactly how it is supposed to be used, then maybe adding an API to cover use case we don't fully understand may be a dead end and will require some adjustments in the future (this may result a BC break). It is already possible to adjust date formats by overriding translations, so you don't even need dedicated extension, you can use Linguist for that. So dateTimeFormats is required to cover some edge case we don't know right now?
Personally I don't like that it do not involve currently used locale - it is easy to forget the fact that formats may differ for different languages. Passing locale to formatCallback() would at least suggest to this is something you should care about.

@YUCLing Can we have this PR against 1.x branch?

@rob006
Copy link
Contributor

rob006 commented Oct 3, 2024

OK, I checked new implementation and now I'm even more confused. I assumed that formatCallback() will get translation key (formatCallback('core.lib.datetime_formats.human_time_short')), so it could be used to overwrite translation for specific context. But it looks like it gets format taken from translation (formatCallback('ll')) - I have no idea what is practical use case for this.

@SychO9
Copy link
Member

SychO9 commented Oct 3, 2024

@rob006 but I gave you a simple valid use case. For the user to be able to fill the formats themselves as a preference. I understand it is not something you personally see the point in. This is something I can do in other forum software as well and isn't all that odd.

Also, even without this a developer could still make it happen. But by modifying parts of the code that we consider private API. What we add here is a public API where even if we make changes to the code in the future, allows us to prevent BC breaks by handling the item list as necessary.

@rob006
Copy link
Contributor

rob006 commented Oct 3, 2024

For the user to be able to fill the formats themselves as a preference. I understand it is not something you personally see the point in. This is something I can do in other forum software as well and isn't all that odd.

And IMO this is a wrong place to cover this use case. I does not allow you to remove "2 minutes ago" format and use precise date instead (for example I can do this in phpBB). I we want the ability to change date format used by forum it makes more sense to override humanTime() instead of adding some hooks in formatDateTime().

@rob006
Copy link
Contributor

rob006 commented Oct 3, 2024

BTW: #4053 just got merged to 1.x branch, so I guess there is no point to rebase this PR and we can leave 2.x as target.

@SychO9
Copy link
Member

SychO9 commented Oct 3, 2024

And IMO this is a wrong place to cover this use case. I does not allow you to remove "2 minutes ago" format and use precise date instead (for example I can do this in phpBB). I we want the ability to change date format used by forum it makes more sense to override humanTime() instead of adding some hooks in formatDateTime().

Sorry but that's just a different use case. It does not invalidate the simple use case of just wanting wanting to directly replace a specific format for a raw format value, which is possible regardless and is just a matter of how best to allow it being done.


Shouldn't we do the same for other formats in the codebase? I see a lot .format('LLLL') and .format('LLL') for example.

@rob006
Copy link
Contributor

rob006 commented Oct 3, 2024

Sorry but that's just a different use case.

How is this different use case? If the goal is to allow customization of the date format used in the forum by a specific user, then dateTimeFormats would allow for the implementation of a half-way solution at best. Do we really need 3 official ways of adjusting date formats (1. by translation for simple cases, 2. by overriding humanTime() for advanced scenarios, 3. dateTimeFormats for something in the middle)?

@SychO9
Copy link
Member

SychO9 commented Oct 3, 2024

I assumed that formatCallback() will get translation key

It should by the way. There is no point if it doesn't. Probably just a mistake.

Do we really need 3 official ways of adjusting date formats (1. by translation for simple cases, 2. by overriding humanTime() for advanced scenarios, 3. dateTimeFormats for something in the middle)?

You can't override functions in flarum, so I'm not following how that's related.

I'm not sure I understand why you are against the use case of: as a user I want to be able to customize the scrubber date format.

@YUCLing
Copy link
Contributor Author

YUCLing commented Oct 4, 2024

Shouldn't we do the same for other formats in the codebase? I see a lot .format('LLLL') and .format('LLL') for example.

My bad, I didn't search in tsx files.

Also, the formats in admin panel is currently not considered.

@SychO9
Copy link
Member

SychO9 commented Oct 22, 2024

@YUCLing I've pushed some changes, mainly to pass the ID to the callbacks as that's more useful than a changing final format. Can you check the changes and let me know if you're good to merge this?

@YUCLing
Copy link
Contributor Author

YUCLing commented Oct 23, 2024

@SychO9 LGTM

@SychO9 SychO9 added this to the 2.0 milestone Oct 23, 2024
@SychO9 SychO9 merged commit d041515 into flarum:2.x Oct 23, 2024
18 checks passed
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.

Chinese month and year expression of Scrubber-Description element cloud be better
4 participants