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: add calendar locales for nl #570

Closed
wants to merge 3 commits into from
Closed

Conversation

RWOverdijk
Copy link

This PR adds translations for the Calendar plugin when the locale is set to Dutch.

So, this works only for NL (missing languages will still fall back to en). Also, this only works with the LocalizedFormat plugin enabled. I don't expect this PR to be merged. But I do hope it starts a conversation.

I have some separate concerns/issues I'd like to address as well.

1. Configure plugins

Right now, the dates in calendar are hard to use when combined with other locales. I'd have to pass in the format with every call I make, which is not great.

What I mean by that is that the calendar plugin does use translations. However, as soon as you pass in 1 option, it completely ignores the translations:

dayjs(date).calendar(dayjs())

☝️ Uses the translations from the locale.

dayjs(date).calendar(dayjs(), {
  sameElse: 'L LT'
});

☝️ This will now no longer read the translations from the locale for the other options. This is the offending line causing that behaviour:

const format = formats || this.$locale().calendar || calendarFormat

By allowing the calendarFormat to be configured once, you remove both the need to supply the options on each call and the bug that ignores translations.

Something like this perhaps, which is also easy to keep backwards compatible:

dayjs.extend(Calendar.factory({ defaults: 'go here' }));

2. Translations for calendar

I think the calendar should construct its options the same way RelativeTime does it.

This would solve half of above issue as well and would be more in line with the way locales work.

3. Translations

One example where literal translations aren't enough is Portuguese.

lastWeek has passada for weekdays whereas sábado and domingo would be passado. Weekends are male.

Perhaps those could optionally be made into functions? That way these cases will still work.

4. Plugins aware of plugins

For the calendar it would make sense to depend on, or be aware of the LocalizedFormat plugin. It uses localized formats that would otherwise be duplicated. Considering the small size of the plugin I'd say it's not a strange thing to move.

Adding the translations for calendar to locales would already be a huge win (which is what this PR does for Dutch).

Conclusion

Sorry for all the stuff I'm asking you to read. But I do have the best of intentions. This library rules! 😄

@codecov-io
Copy link

codecov-io commented Apr 15, 2019

Codecov Report

Merging #570 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #570   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files       154    154           
  Lines       966    968    +2     
  Branches    131    131           
===================================
+ Hits        966    968    +2
Impacted Files Coverage Δ
src/locale/nl.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f5a9db...8419b61. Read the comment docs.

@RWOverdijk
Copy link
Author

I've updated the PR to use single quotes, and work without needing the LocalizedFormat making it a safe addition. 😄

@iamkun iamkun added the discussion Further discussion label Apr 15, 2019
@iamkun
Copy link
Owner

iamkun commented Apr 15, 2019

Thanks for this PR. I'm not going to merge it until we have a final decision.
I think we should leave this to our users based on their every different project. That's the main reason I'm not including the calendar part to locale files.
Discussion is welcomed.

Just a quick replay for the first issue:
You could simply do

import 'dayjs' from 'dayjs';
import en from 'dayjs/locale/en';
dayjs.locale({
    ...en,
    calendar: {
      sameElse: 'YYYY-MM-DD'
    }
});

to override the build-in locale with your custom part. In this way, you don't have to pass the calendar translation to every call you make. Ref: #215

@RWOverdijk
Copy link
Author

That's good to know. I am doing that now, sort of.

Regarding adding translations, the ones for localized are part of the locales, shouldn't those be taken out too then? It's about 5 keys in an object so I can't think of any reason to let everyone define their own keys rater than including it with locales.

Perhaps I'm being naive, but that's why discussions are good. 😄

@iamkun
Copy link
Owner

iamkun commented Apr 16, 2019

@RWOverdijk That's another interesting thing. If we put everything to the locale file, the size of the locale file would become huge. Imaging some user just wants to use the main part of the locale (weekday and month), the locale of relative time calendarplugins would be useless and unshakable.

That not blame to this PR but maybe the whole project design at the very beginning.

@RWOverdijk
Copy link
Author

@iamkun What's this "everything" though? There aren't that many plugins (that require translations).

When combined the boilerplate size goes down. Right now there's a bunch of boilerplate for environment detection that's larger than for example the translations for calendar. Also, when combining them you get to reuse characters for localized for example ('L' instead of 'MM/DD/YYYY'), so the size argument wouldn't work here I think.

Another option might be to focus on my first point about factories. If plugins can patch the locales in dayjs themselves, then they can bundle their own locales.

The obvious downside to this is that you'd have distributed locale files, many of them, rather than having it all in one place. Also the filesize would go up for anyone using at least one plugin due to the boilerplate.

By boilerplate, I mean this by the way:

!function(e, t) {
  'object' == typeof exports && 'undefined' != typeof module ? module.exports = t(require('dayjs')) : 'function' == typeof define && define.amd ? define(['dayjs'], t) : e.dayjs_locale_ar = t(e.dayjs);
}(this, function(e) {
  'use strict';
  e = e && e.hasOwnProperty('default') ? e.default : e;
    /* code */
});

which is 331 characters, whereas this PR for example only adds 215.

@iamkun
Copy link
Owner

iamkun commented Apr 16, 2019

What I'm wondering now is that should each plugin has its own locale file, so that we could load everything on demand. But this is still not a good way since we would be facing hundreds of locale files, and hard to use and manage.

However, if we still put plugin's locale into the current locale, the size of the locale file will increase after we have more plugins later.

Hope you could understand my concerns. And we could be looking for a better option (if there is one).

@RWOverdijk
Copy link
Author

@iamkun I understand completely, it's what I think I addressed in my previous comment, too. It's a library with almost 21k stars, you can't just go messing around with it 😄

I do honestly think that the size argument is one that's not really a big deal because it'll add very little.

@ghost
Copy link

ghost commented Apr 16, 2019

I like Day.js just because of its tiny size. If we really put "every thing" into Day.js it will surely become next moment.js. That's not what we want and not the author want.

It's just like magic that a 21k starts popular library could control its size around 3kb.

Adding is easy, removing is hard.

@tomhuze
Copy link

tomhuze commented Aug 27, 2019

The project I am working on only supports ~12 locales, so extending each of those into a custom locale with calendar formatting seems like a very reasonable solution. While it would easier for my use case if the plugins had their own locale files, I can certainly understand the maintenance headaches for the dayjs project for that solution, or alternatively the frustration of someone supporting 100+ locales and not using a lot of plugins with the file size impact if all current (and future?) plugin-related formats were included in the core locale files.

I guess the bigger question is: are the bulk of users supporting just a few locales and using a variety of plugins, in which case the benefits of including the plugin formats in the locales seems to outweigh the costs; or are they supporting most/all of the available locales and mostly just using the plugins which formats are already included in the locale files, in which the present set up seems like the best solution?

@iamkun
Copy link
Owner

iamkun commented Aug 27, 2019

There might be a simple solution, just provide two locale file for each language. One full version and the other only the minimum like month and week data.

@iamkun
Copy link
Owner

iamkun commented Apr 7, 2020

Please use UpdateLocale plugin.

https://day.js.org/docs/en/customization/customization

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

Successfully merging this pull request may close these issues.

4 participants