-
Notifications
You must be signed in to change notification settings - Fork 4.6k
DataViews: use local time in summary filter and default render for datetime fields #72756
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
Conversation
| <RenderFromElements item={ item } field={ field } /> | ||
| ) : ( | ||
| field.getValue( { item } ) | ||
| new Date( field.getValue( { item } ) ).toLocaleString() |
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.
Perhaps this should be done in the default getValue generated for the field type: by doing that, it won't interfere with the getValue provided by the field authors that may have already dealt with this.
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 okay as it is: field authors can provide its own render. getValue is focused on data retrieval and render on data display. By doing this, we enable field authors to provide its own getValue to retrieve the data from whenever it is, but they still benefit from the default render in local time.
| return filterInView?.value?.includes( element.value ); | ||
| } ); | ||
| } else if ( filterInView?.value !== undefined ) { | ||
| // Find the field to check its type |
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 a proof of concept. Consider using getValue from the field (see note below) instead. Note the filter's input-widget prepares the field in a specific way (because it doesn't have access to any item).
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 good as it is, see #72756 (comment)
5036924 to
e63c659
Compare
|
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. |
|
This is a good first step to provide good defaults. We need to work on enabling field authors to provide their own timezone, in case they want to use settings from the environment (WordPress, etc.) — that should be used by the filter summary, render, and Edit functions. |
|
Flaky tests detected in 5ae96ea. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18901222701
|
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.
Left some comments for consideration but non blockers, things tested well for me 👍
| <RenderFromElements item={ item } field={ field } /> | ||
| ) : ( | ||
| field.getValue( { item } ) | ||
| new Date( field.getValue( { item } ) ).toLocaleString() |
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.
Here we creating a date without any try catch, while above we have a try catch, I guess we should also add one here.
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.
Right now, if an invalid data is returned by field.getValue( { item } ), I see "invalid date" as the field's value. This is the same for the date field too. If we add a try-catch here to return field.getValue( { item } ) in case it is not a date, then let's make this consistent in the date field too.
|
|
||
| if ( field?.type === 'datetime' && typeof label === 'string' ) { | ||
| try { | ||
| label = new Date( label ).toLocaleString(); |
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.
If label is null it will return the epoch (Jan 1, 1970), not sure if we want to handle this case as we are using ts and label is not expected to be null.
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.
Yeah, label won't be null at this point because we check for its type above (type of null would be object so it won't go into this conditional).
| <RenderFromElements item={ item } field={ field } /> | ||
| ) : ( | ||
| field.getValue( { item } ) | ||
| new Date( field.getValue( { item } ) ).toLocaleString() |
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.
If the date is undefined we will render "Invalid date" not sure if we want to render an empty string instead?

When the date is empty it is not that it is invalid, it may even be expected, but I guess we can also leave to the consumers the decision on how to handle this case, as they are the ones who know the meaning of an empty date.
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 agree -- an empty date could be used to represent the TBA/TBD state.
|
Looking good @oandregal ! I tested this on Storybook while being in a UTC + 2 timezone and confirmed that date/time values are now consistent:
and filtered results are returned correctly:
I had some thoughts around the "invalid date" state, but @jorgefilipecosta had already raised all of them so I followed-up on the existing comments. I also ran the PR changes by Cursor for an extra 👀 and the only thing that came up was adding a try/catch block here, which was already mentioned. |
|
Thanks for the reviews @jimjasson and @jorgefilipecosta I've pushed 94465de to better handle invalid dates, and the render method displays empty for them. I think this was all, so I'll set this to auto-merge when tests pass. Please, be welcome to leave a review if there's something you'd like me to address — even if it's post-merge. |


Fixes #72659
What?
Displays the
datetimefield using the local timezone.Why?
There is a discrepancy in how we displayed
datetimefields:datetimetype displays the datetime as it is (no transformation to local timezone)How?
This switches both the filter summary and the default render function for the
datetimeto using the local timezone as well.Testing Instructions
npm run storybook:dev).