picker
picker copied to clipboard
feat: add support for luxon
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
getShortWeekDaysimplementation 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
getWeekFirstDayfor 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.
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 Report
Merging #230 (b190ded) into master (13bb773) will increase coverage by
0.01%. The diff coverage is100.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
Great PR. Any chance we can get this merged?
@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 π
@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.
Could you merge PR? @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
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
π’
Bumping this. Would love support for luxon!
Great PR. Would really like to get it merged as luxon is more reliable than date-fns when dealing with timezones
Possible to merge this PR please ?
What is the status of this PR? Would be great to have this!
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) |
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 π
Thanks a lot @zombieJ!
I'd be happy to help with updating the documentation in antd if that is something you're interested in.
Are peerDependencies and peerDependenciesMeta missing?
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