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

Improve documentation for dateI18n. Standardise way to output date. Add function dateI18nAddTimezone. #26276

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Oct 19, 2020

This PR improves documentation for dateI18n.
Checks the codebase and standardizes the ways we output dates, mostly relying on wp.date. dateI18nAddTimezone now. This change fixes an issue that packages/block-library/src/post-date/edit.js and packages/block-library/src/post-comment-date/edit.js had similar to the one fixed in #26212.

We also fix an issue that was present in multiple blocks when outputting in a format that has a timezone representation (e.g., the 'c' format very used to output time HTML elements). The timezone used was the local one instead of the WordPress one, and the value was converted to the website timezone, making the time wrong because the date was already on the website timezone.
What we needed to output time HTML elements and to general date formats, in general, is a function that receives the time in the WordPress timezone (but whose time does not specify the timezone) adds the WordPress timezone keeping the same hours.
E.g., if we receive a string "2020-10-16T14:36:58", the website is on -4 UTC offset, and the user is on +2 UTC offset when applied the "c" format we want to output:
2020-10-16T14:36:58-04:00

If we call wp.date.format('c', '2020-10-16T14:36:58'); we get `2020-10-16T14:36:58+02:00. The hours are not changed; that's why the fix in #26212 worked, but the timezone specified is not correct. It is the user timezone, and it should be the website timezone. Browsers would announce the time on the time elements incorrectly.

If we call wp.date.dateI18n('c', '2020-10-16T14:36:58'); we get 2020-10-16T08:36:58-04:00. The output is on the website timezone but the hours are not correct because they were converted without need.

If we call wp.date.dateI18nAddTimezone('c', '2020-10-16T14:36:58'); we get the correct output 2020-10-16T14:36:58-04:00.

How has this been tested?

On the browser console, I made some tests to wp.date.dateI18nAddTimezone calls and verified the results were the expected ones.
I added the latest posts block, post date block, and post comment date blocks. And I verified the time element they output (using the browser inspector) is in the website timezone and matches exactly what is output on the frontend by the server. Previously the outputs between server and client did not match.

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Package] Date /packages/date labels Oct 19, 2020
@github-actions
Copy link

github-actions bot commented Oct 19, 2020

Size Change: +92 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/block-directory/index.js 8.72 kB -1 B
build/block-editor/index.js 134 kB -4 B (0%)
build/block-library/index.js 148 kB +16 B (0%)
build/blocks/index.js 48.1 kB +1 B
build/components/index.js 172 kB -5 B (0%)
build/date/index.js 11.3 kB +71 B (0%)
build/edit-post/index.js 306 kB +2 B (0%)
build/edit-site/index.js 23.6 kB +2 B (0%)
build/edit-widgets/index.js 26.3 kB +1 B
build/editor/index.js 43.3 kB +8 B (0%)
build/redux-routine/index.js 2.84 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/editor-rtl.css 8.95 kB 0 B
build/block-library/editor.css 8.95 kB 0 B
build/block-library/style-rtl.css 8.23 kB 0 B
build/block-library/style.css 8.23 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.95 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 8.8 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.95 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/style-rtl.css 6.42 kB 0 B
build/edit-post/style.css 6.4 kB 0 B
build/edit-site/style-rtl.css 3.92 kB 0 B
build/edit-site/style.css 3.92 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.27 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.81 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/reusable-blocks/index.js 2.92 kB 0 B
build/rich-text/index.js 13.3 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.05 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@jorgefilipecosta jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Oct 19, 2020
@jorgefilipecosta jorgefilipecosta force-pushed the update/improve-documentation-for-dateI18n-function-consolidate-date-formats-to-the-format-function-instead-of-gmdateI18n-and-dateI18n branch from 8a1666a to b6fd3de Compare October 19, 2020 22:27
@jorgefilipecosta jorgefilipecosta changed the title Improve documentation for dateI18n. Standardise way to output date. Fix timezone issue on wp.date.format. Improve documentation for dateI18n. Standardise way to output date. Add function dateI18nAddTimezone. Oct 19, 2020
@jorgefilipecosta jorgefilipecosta force-pushed the update/improve-documentation-for-dateI18n-function-consolidate-date-formats-to-the-format-function-instead-of-gmdateI18n-and-dateI18n branch from b6fd3de to 8c8dd8a Compare October 19, 2020 22:42
@jorgefilipecosta jorgefilipecosta removed the [Status] In Progress Tracking issues with work in progress label Oct 20, 2020
@jorgefilipecosta jorgefilipecosta force-pushed the update/improve-documentation-for-dateI18n-function-consolidate-date-formats-to-the-format-function-instead-of-gmdateI18n-and-dateI18n branch from 8c8dd8a to c9b26a6 Compare October 20, 2020 11:31
@mcsf
Copy link
Contributor

mcsf commented Oct 21, 2020

Would it be possible to fix the issue by, instead, ensuring that date values coming from the REST API always have the appropriate timezone attached?

@jorgefilipecosta
Copy link
Member Author

Would it be possible to fix the issue by, instead, ensuring that date values coming from the REST API always have the appropriate timezone attached?

Hi @mcsf,

Yes having the API return the timezone, and making sure our components that produce dates, (post schedule component, post date block, etc) return dates with an attached timezone too, and making sure we always format dates with the existing dateI18n function would also be a valid fix.

The question is if changing the time format returned by the WordPress API is a breaking change or not? Is it something we can do and should consider doing? cc: @TimothyBJacobs as someone very involved in rest API work in case you have some thoughts on this.
The REST also returns all the time fields with a _gmt suffix and with the dates in GMT. The problem is currently we can not rely on this field because all of our components that change dates never update this field. Another option we also have would be to rely on the GMT field and make sure all the components that manipulate dates also property update the _gtm suffixes.

@TimothyBJacobs
Copy link
Member

Relevant core ticket: https://core.trac.wordpress.org/ticket/39855

I think it'd probably be considered a breaking change. The GMT offset is exposed in the index, so you could manually change the date fields in the client to include the timezone I suppose?

@vcanales vcanales self-requested a review November 24, 2020 20:22
@talldan
Copy link
Contributor

talldan commented Nov 25, 2020

Does the PR fix this issue? #27251

@jorgefilipecosta jorgefilipecosta force-pushed the update/improve-documentation-for-dateI18n-function-consolidate-date-formats-to-the-format-function-instead-of-gmdateI18n-and-dateI18n branch from c9b26a6 to 6b09064 Compare November 25, 2020 11:14
@jorgefilipecosta
Copy link
Member Author

Hi @talldan, yes this PR should fix the issue. But before we can merge this PR, we need to fix the issues shown on #27272.

Base automatically changed from master to trunk March 1, 2021 15:44
@youknowriad youknowriad deleted the update/improve-documentation-for-dateI18n-function-consolidate-date-formats-to-the-format-function-instead-of-gmdateI18n-and-dateI18n branch July 22, 2021 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Date /packages/date [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants