feat: add support for luxon #230

hihuz · 2021-03-11T17:15:10Z

ref issues: ant-design/ant-design#29710 & ant-design/ant-design#22732

Description

First of all, huge thank you to the maintainers of this library, and any time spent reviewing this PR is greatly appreciated 🙂

The idea is to provide an integration for luxon in the rc-picker.

For most of the methods, luxon has moment equivalents, but in rare cases there are some differences that I will list below.
For the most part it comes from the fact that luxon strictly uses the standard Intl API for localisation.

As of now, this API does not have any notion of localised weeks (it uses ISO weeks for everything), and it features some differences in terms of formatting (notably for the list of shortened day names).

In practice the differences will be:

First day of the week is always Monday regardless of locale
Week of year number is sometimes different, as the rule for ISO Weeks is:

The first week of the year, hence, always contains 4 January.

Short week days will sometimes be different (see below for details).
luxon has no "human readable" format for weeks (e.g. moment has "2012-51st" but luxon hasn't)

Short week days differences

luxon has two possible week day formats that we can use: "short" or "narrow".

"short" is usually a bit longer than the moment weekdaysMin() equivalent (usually 3 characters), and "narrow" is usually just 1 character.

I added some custom logic in my implementation to keep the same week day format as moment for the main antd locales (the same ones that we have in the dayjs.ts config: en_GB, en_US, zh_CN, zh_TW), but I didn't want to bloat the implementation with formatting for all the locales.

I can think of two options to allow users to customize it further:

We can keep the current implementation and simply document how to override the formats, but the getShortWeekDays implementation is slightly complex so users might not want to deal with that.
Instead of a static config object, we could implement a function returning the static config object which would accept a { weekDayFormat, weekDayLength } options object. We would then export both the "default" config, and the function to create a config with different formats and lengths, e.g. something like

const configGenerator = ({ weekDayFormats, weekDayLengths }: ConfigGeneratorOptions): GenerateConfig<DateTime> =>  ({
  ...
});
...

const generateConfig = configGenerator({ weekDayFormats: defaultWeekDayFormatMap, weekDayLengths: defaultWeekDayLengthMap });

export { configGenerator };
export default generateConfig;

I think this is the better solution, and I would be happy to update the PR as required, but I didn't want to diverge too much from the other configs for the initial implementation.

Start of week differences

As mentioned, with ISO weeks, a week start on Monday for all locales. This can be suboptimal, and it is possible to override this luxon default, but doing so breaks the "week" picker (see below for details).

Similarly to week days above, we could either:

Document how to override getWeekFirstDay for users who want a different starting day for their weeks.
Instead of a static config object, we could implement a function returning the static config object which would accept a { firstWeekDays } options object. We would then export both the "default" config, and the function to create a config with different formats and lengths (for a sample implementation, see above).

Users need to be aware that doing such an override would break the week picker, so we definitely want to document that.

Week of year differences

As mentioned, the week of year number for a given day can be different, as luxon only deals with ISO weeks and not locale weeks.

First, it can happen because the first week of the year is not always the same, and second it can happen because ISO weeks don't always start on the same day as locale weeks.

For this second point, if users override the start day of the week for their locale as indicated above, they will be in a situation where "visually" their week starts for example on Sunday, but getWeek for that Sunday will return the previous week, and it would make the "week" picker unusable.

There is no way around this unless we want to manually rewrite the logic to retrieve week numbers and week days, and I think this would be a bit too much, especially since only the "week" picker would be impacted, and I believe luxon users would already be aware of this ISO week situation and limitation in the first place.

So I believe some documentation for this point is enough.

Selected week label formatting

luxon doesn't provide "human readable" formatting (e.g. 1st, 2nd, 3rd...), and this format is used to display the selected week in the week picker.

Instead of such format, the luxon powered picker will display a basic 01, 02, 03...

As a concrete example: "2021-1st" (moment version) will be displayed "2021-01" with the luxon picker, I thought this was acceptable.

vercel · 2021-03-11T17:15:16Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/react-component/picker/AJ3HNRyNQvuXb42Dv6bqqsMEqbxW
✅ Preview: https://picker-git-fork-hihuz-feature-luxon-support-react-component.vercel.app

codecov · 2021-03-11T17:18:48Z

Codecov Report

Merging #230 (b190ded) into master (13bb773) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head b190ded differs from pull request most recent head 555c557. Consider uploading reports for the commit 555c557 to get more accurate results

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   99.33%   99.35%   +0.01%     
==========================================
  Files          49       50       +1     
  Lines        2257     2309      +52     
  Branches      660      664       +4     
==========================================
+ Hits         2242     2294      +52     
  Misses         13       13              
  Partials        2        2

Impacted Files	Coverage Δ
src/generate/luxon.ts	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

hihuz · 2021-03-11T17:28:44Z

src/generate/luxon.ts

+      const weekdays = Info.weekdays(weekDayFormatMap[locale] || 'short', {
+        locale: normalizeLocale(locale),
+      });
+
+      const shifted = weekdays.map(weekday => weekday.slice(0, weekDayLengthMap[locale]));


As mentioned in the PR description, luxon (through Intl API) formatting for week days does not match moment's 1:1.

I made some static adaptations so that the main antd locales are formatted the same, but I didn't think it was proper to bloat the implementation with formatting for all locales.

I have indicated in the PR description how we could provide easier customization for users, or simply document how to do it with the current implementation.

- The luxon mapping was copied from react-component/picker#230

* spike: add luxon based config generator - The luxon mapping was copied from react-component/picker#230 * spike: fix format string difference Luxon behaves different with format strings. ```tsx const l = DateTime.fromFormat("2021.07", "y.MM.dd") // results in an invalid luxon date l.toFormat("y.MM.dd") // Invalid DateTime const m = moment("2021.07", "y.MM.dd"); // results in an valid moment date m.format("y.MM.DD") // "2021.07.01" ``` * spike: add default value It might not be functional necessary but it should improve readablity. * spike: use correct luxon format token In luxon `DD` means localized date with abbreviated month and `dd` day of the month, padded to 2. * spike: correct naming * spike: remove `firstDayOfWeek` code Currently luxon does not support setting first day of the week (moment/luxon#373) therefore indefinitely removing it. * spike: use `Time` from `@dendronhq/common-all` package `Time` uses `luxon`. * spike(calendar-view): inline defaults There is a case where `config` is `undefined` and then throws at `getMaybeDatePortion`. Until that is fixed we assume default values locally even though they should be defined only in `packages/engine-server/src/config.ts`. * spike: remove `moment` import * spike: fix typescript errors This removes code from the originator. The part that mappes `getShortWeekDays` to starting with Sunday actually rendered a week starting with Monday. Now, with the code removed, it starts with Tuesday. An issue I still need to fix. * spike: move non-page file outside `pages/vscode` nextjs interprets files inside `pages` as actual web pages which `luxon.ts` is not. * spike: display correct weekdays Luxon starts week with Monday indexed as 1. We need to shift so that 1 points to Monday and not Tuesday * chore: fix typo

johannespfeiffer · 2021-10-25T07:35:42Z

Great PR. Any chance we can get this merged?

afc163 · 2021-10-26T11:10:57Z

hihuz · 2022-01-31T16:15:05Z

@afc163 Thank you for the interest!

I resolved the conflicts and bumped luxon to the recently introduced 2.x major version.

The caveat and details explained the original PR's description still apply.

When you get the chance, let me know if I can do anything to help get the PR reviewed and merged 🙏

jamealg-jv · 2022-05-11T16:10:22Z

@afc163 Also looking for Luxon support. Would love to have this. Let us know if there is anything else that can be done to move this along.

honia19 · 2022-06-14T07:48:01Z

Could you merge PR? @hihuz

hihuz · 2022-06-14T08:10:46Z

Could you merge PR? @hihuz

Sorry, but no, I can't 😉 I'm just a first time contributor, the PR would need to be reviewed and merged by a maintainer

honia19 · 2022-06-14T12:46:18Z

Could you merge PR? @hihuz

Sorry, but no, I can't 😉 I'm just a first time contributor, the PR would need to be reviewed and merged by a maintainer

😢

davidgtu · 2022-09-07T17:57:11Z

Bumping this. Would love support for luxon!

elvinasv · 2022-10-11T07:39:51Z

Great PR. Would really like to get it merged as luxon is more reliable than date-fns when dealing with timezones

ap705 · 2022-12-14T14:04:10Z

Possible to merge this PR please ?

arnoutj · 2023-03-01T16:04:47Z

What is the status of this PR? Would be great to have this!

vercel · 2023-03-30T07:24:23Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
picker	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Mar 30, 2023 at 7:32AM (UTC)

hihuz · 2023-03-30T07:31:32Z

I just rebased the PR to fix conflicts, and bumped luxon to the latest major (3.x).

Let's see if we can get the maintainer's attention. 😉

@afc163 @zombieJ any chance you could take a look at this PR if you are still interested in the luxon integration? Much appreciated 🙏

hihuz · 2023-03-30T15:59:20Z

Thanks a lot @zombieJ!

I'd be happy to help with updating the documentation in antd if that is something you're interested in.

zombieJ · 2023-03-30T16:01:40Z

+ rc-picker@3.4.0

yoyo837 · 2023-04-01T07:20:45Z

Are peerDependencies and peerDependenciesMeta missing?

hihuz · 2023-04-01T13:46:56Z

Ah, my mistake, indeed you are correct @yoyo837

These peerDependency fields weren't present when I originally created the PR and missed the change in package.json structure in my last rebase.

I have created a follow-up PR to address the issue: #611

vercel bot deployed to Preview March 11, 2021 17:15 View deployment

vercel bot deployed to Preview March 11, 2021 17:28 View deployment

hihuz commented Mar 11, 2021

View reviewed changes

hihuz mentioned this pull request Mar 15, 2021

luxon integration for pickers ant-design/ant-design#29710

Closed

1 task

namjul mentioned this pull request Jun 10, 2021

feat: calendar panel dendronhq/dendron#806

Merged

namjul pushed a commit to dendronhq/dendron that referenced this pull request Jul 29, 2021

spike: add luxon based config generator

1f6891b

- The luxon mapping was copied from react-component/picker#230

namjul pushed a commit to dendronhq/dendron that referenced this pull request Aug 2, 2021

spike: add luxon based config generator

5ec3edb

- The luxon mapping was copied from react-component/picker#230

hihuz force-pushed the feature/luxon-support branch from 65d11d1 to 5fc53e1 Compare January 31, 2022 16:09

vercel bot deployed to Preview January 31, 2022 16:09 View deployment

hihuz added 3 commits March 30, 2023 09:19

feat: add support for luxon

209875a

fix: remove inconsistent formatting changes

2c9be2e

feat: bump luxon version to 2.x

341f71b

hihuz force-pushed the feature/luxon-support branch from 5fc53e1 to b5fa90f Compare March 30, 2023 07:24

vercel bot deployed to Preview March 30, 2023 07:26 View deployment

feat: bump luxon version to 3.x

555c557

hihuz force-pushed the feature/luxon-support branch from b5fa90f to 555c557 Compare March 30, 2023 07:29

vercel bot deployed to Preview March 30, 2023 07:32 View deployment

zombieJ merged commit e2588e8 into react-component:master Mar 30, 2023

hihuz deleted the feature/luxon-support branch March 30, 2023 15:58

hihuz mentioned this pull request Mar 31, 2023

Picker luxon support ant-design/ant-design#41580

Merged

20 tasks

hihuz mentioned this pull request Apr 1, 2023

fix: add missing luxon peer dependency #611

Merged

afc163 mentioned this pull request Apr 12, 2023

Support luxon as the date format #580

Closed

Uh oh!

feat: add support for luxon #230

feat: add support for luxon #230

Uh oh!

Conversation

hihuz commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Short week days differences

Start of week differences

Week of year differences

Selected week label formatting

Uh oh!

vercel bot commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hihuz Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

johannespfeiffer commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

afc163 commented Oct 26, 2021

Uh oh!

hihuz commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamealg-jv commented May 11, 2022

Uh oh!

honia19 commented Jun 14, 2022

Uh oh!

hihuz commented Jun 14, 2022

Uh oh!

honia19 commented Jun 14, 2022

Uh oh!

davidgtu commented Sep 7, 2022

Uh oh!

elvinasv commented Oct 11, 2022

Uh oh!

ap705 commented Dec 14, 2022

Uh oh!

arnoutj commented Mar 1, 2023

Uh oh!

vercel bot commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hihuz commented Mar 30, 2023

Uh oh!

hihuz commented Mar 30, 2023

Uh oh!

zombieJ commented Mar 30, 2023

Uh oh!

yoyo837 commented Apr 1, 2023

Uh oh!

hihuz commented Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

hihuz commented Mar 11, 2021 •

edited

Loading

vercel bot commented Mar 11, 2021 •

edited

Loading

codecov bot commented Mar 11, 2021 •

edited

Loading

johannespfeiffer commented Oct 25, 2021 •

edited

Loading

hihuz commented Jan 31, 2022 •

edited

Loading

vercel bot commented Mar 30, 2023 •

edited

Loading

hihuz commented Apr 1, 2023 •

edited

Loading