picker icon indicating copy to clipboard operation
picker copied to clipboard

feat: add support for luxon

Open hihuz opened this issue 4 years ago β€’ 11 comments

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.

hihuz avatar Mar 11 '21 17:03 hihuz

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

vercel[bot] avatar Mar 11 '21 17:03 vercel[bot]

Codecov Report

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

:exclamation: 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%> (ΓΈ)

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

codecov[bot] avatar Mar 11 '21 17:03 codecov[bot]

Great PR. Any chance we can get this merged?

johannespfeiffer avatar Oct 25 '21 07:10 johannespfeiffer

图片

afc163 avatar Oct 26 '21 11:10 afc163

@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 πŸ™

hihuz avatar Jan 31 '22 16:01 hihuz

@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.

jamealg-jv avatar May 11 '22 16:05 jamealg-jv

Could you merge PR? @hihuz

honia19 avatar Jun 14 '22 07:06 honia19

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

hihuz avatar Jun 14 '22 08:06 hihuz

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 avatar Jun 14 '22 12:06 honia19

Bumping this. Would love support for luxon!

davidgtu avatar Sep 07 '22 17:09 davidgtu

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

elvinasv avatar Oct 11 '22 07:10 elvinasv

Possible to merge this PR please ?

ap705 avatar Dec 14 '22 14:12 ap705

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

arnoutj avatar Mar 01 '23 16:03 arnoutj

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)

vercel[bot] avatar Mar 30 '23 07:03 vercel[bot]

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 avatar Mar 30 '23 07:03 hihuz

Thanks a lot @zombieJ!

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

hihuz avatar Mar 30 '23 15:03 hihuz

+ [email protected]

zombieJ avatar Mar 30 '23 16:03 zombieJ

Are peerDependencies and peerDependenciesMeta missing?

yoyo837 avatar Apr 01 '23 07:04 yoyo837

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: https://github.com/react-component/picker/pull/611

hihuz avatar Apr 01 '23 13:04 hihuz