-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
TimePicker: Add dateOrder
prop to sort day, month, and year
#62481
Conversation
Size Change: +236 B (+0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working well for me and I believe it is a good compromise that allows some flexibility while still remaining stable enough.
For a moment, I wondered whether "date order" is a good way to name it, but I think it is, because "date format" would be more confusing, as date format fields are usually more flexible and folks could start adding unexpected tokens there.
I'd prefer to defer the full review to @mirka who has recently been working on some of the time and date components.
Co-authored-by: Lena Morita <lena@jaguchi.com>
@mirka Thanks for the review! I have tried to maintain the default behavior of the component based on feedback. 3ece19e02dd3e268fef79ca19d60e355.mp4I also added another unit test and clarified the documentation. |
is12Hour={ is12HourFormat( | ||
siteTimeFormat | ||
) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this prop is no longer needed as the proper time format for the locale is now applied via dataOrder
prop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I missed that 😅 Restored the prop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a few suggestions, but otherwise looks good! Thanks for the great enhancement 🙏
is12Hour={ is12HourFormat( | ||
siteTimeFormat | ||
) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Lena Morita <lena@jaguchi.com>
While testing this PR again, I discovered that the default value for the Beforebefore.mp4Afterafter.mp4 |
Looks good 👍 |
P.S. When I resolved the conflict, I noticed that the |
…ess#62481) * TimePicker: Add `dateOrder` prop to sort day, month, and year * Add changelog * Update unit tests * Fix grammar * Fix changelog * Refactoring * Update packages/block-library/src/post-date/edit.js Co-authored-by: Lena Morita <lena@jaguchi.com> * Revert unwanted changes * `dateOrder` prop overwrites `is12Hour` prop * Add unit test * Clarify the documentation * Restore unit test * Improve test comment * Remove unnecessary is12Hour prop * Improve doc Co-authored-by: Lena Morita <lena@jaguchi.com> * Simplify logic * Restore `is12Hour` prop * Adjust prop position * Remove `dateOrder` prop default value from `DateTimePicker` --------- Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: carstingaxion <carstenbach@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
…ess#62481) * TimePicker: Add `dateOrder` prop to sort day, month, and year * Add changelog * Update unit tests * Fix grammar * Fix changelog * Refactoring * Update packages/block-library/src/post-date/edit.js Co-authored-by: Lena Morita <lena@jaguchi.com> * Revert unwanted changes * `dateOrder` prop overwrites `is12Hour` prop * Add unit test * Clarify the documentation * Restore unit test * Improve test comment * Remove unnecessary is12Hour prop * Improve doc Co-authored-by: Lena Morita <lena@jaguchi.com> * Simplify logic * Restore `is12Hour` prop * Adjust prop position * Remove `dateOrder` prop default value from `DateTimePicker` --------- Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: carstingaxion <carstenbach@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Closes #57007, #38221
What?
This PR proposes to add a new
dateOrder
prop to theDateTimePicker
andTimePicker
components, allowing the order of days, months, and years to be changed per locale.Why?
The order of day, month, and year may not be appropriate in all locales. For example, in China and Japan, it is natural to always have year, month, and day in that order.
How?
dataOrder
prop aredmy (DD-MM-YY)
,mdy (MM-DD-YY)
, orymd (YY-MM-DD)
. These values are sourced from "List of date formats by country".dataOrder
prop. This allows us to control the order of year, month, and day per locale.dmy (DD-MM-YY
), and if a locale specifies an incorrect format, this default format will be used.This idea is based on the following approaches that have already been implemented:
Testing Instructions
On the block editor
Default Locale
en
).MM-DD-YY
, otherwise it should beDD-MM-YY
.Other locales
YY-MM-DD
.85305dbb2dec1a00c1961137ab73ae25.mp4
On the storybook
Confirm that changing the dateOrder prop swaps the order of the items.
be53454f71cb4b45405280a299c244c2.mp4