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

Add Features doc for new Time settings #3142

Merged
merged 2 commits into from
Aug 22, 2024
Merged

Add Features doc for new Time settings #3142

merged 2 commits into from
Aug 22, 2024

Conversation

philrz
Copy link
Contributor

@philrz philrz commented Aug 21, 2024

No description provided.

@philrz philrz requested a review from jameskerr August 21, 2024 18:27
@philrz philrz self-assigned this Aug 21, 2024
@@ -1,7 +1,3 @@
---
sidebar_position: 2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped these sidebar_position directives in all these Features articles since that way they're automatically sorted alphabetically.

[UTC](https://en.wikipedia.org/wiki/Coordinated_Universal_Time), e.g.,
`2024-08-14T19:12:51.123456789Z`.

Starting with Zui release v1.18.0, two different options in Zui **Settings**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put in the forward-looking reference to the next Zui release so that way I can merge this right away and ping people on community Slack and/or the person that opened brimdata/brimcap#352 to get early feedback.

@@ -37,7 +37,7 @@ export function runConfigurations() {
properties: {
timeZone: {
name: "timeZone",
label: "Timezone",
label: "Time Zone",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I was assembling the screenshot, I realized the two-word "Time Zone" seems to be the more common use in places like RFC 3339 and ISO 8601, so I've switched to that.

@@ -50,7 +50,7 @@ export function runConfigurations() {
placeholder: "%Y-%m-%dT%H:%M:%S.%L%:z",
helpLink: {
label: "docs",
url: "https://github.com/samsonjs/strftime?tab=readme-ov-file#supported-specifiers",
url: "https://zui.brimdata.io/docs/features/Time-Display",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chicken/egg. This link will be broken until the PR merges. C'est la vie!

2. If the **Time Format** setting is changed from its empty default, any
[strftime directives](https://github.com/samsonjs/strftime?tab=readme-ov-file#supported-specifiers)
in the setting are used to format time values. For instance the setting
`%m/%d/%Y %Z` would produce the displayed value `08/14/2024 Pacific Daylight Time`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should use the capital %Z in this example since it's broken in the strftime library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jameskerr: Yeah, you'd mentioned offline how you perceived %Z to be broken and I'd not spotted a problem before putting up this PR. And trying it again now it seems to be working as I'd expect.

For instance, I start by importing this file t.zson:

{ts:2024-08-14T19:12:51.123456789Z}

Here's how it looks with the default ZSON formatting:

image

Here's how it changes if I change just the Time Zone to "US/Pacific" and let the the default strftime formatting show the modified base time with the offset. So, hour 19 changes to hour 12.

image

Now I go back into Settings and also change Time Format to %Y-%m-%dT%H:%M:%S.%L %Z. The base time still shows hour 12 so that seems correct, and the written out time zone communicates what the numeric offset had been doing before.

image

So, that all seems correct to me, but maybe there's something else I'm missing. 😄 If it'd be easier to hash this out interactively on a Zoom just ping me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jameskerr and I spoke offline and I understand what's broken now. Here's some detail for the record.

I wasn't triggering the problem in my last comment above because I picked "US/Pacific" which happens to also be my local time zone. However, if I pick a different time zone like "US/Eastern", we see:

image

That shows the hour has been correctly adjusted to 15 but the time zone shown is still my local one. In other words, the library behaves as if %Z is to "tell me about the time zone I'm in" as opposed to "tell me about the time zone I asked to format in". This seems like a bug. However, I'm guessing that most users that reach for these settings will probably be seeking to display in their local time zone anyway, so they may not even notice.

For now I'll rework the example to avoid showing %Z to avoid drawing unnecessary attention.

Copy link
Member

@jameskerr jameskerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that one example can be updated, but then looks good.

@philrz philrz merged commit 9cdf527 into main Aug 22, 2024
5 checks passed
@philrz philrz deleted the time-docs branch August 22, 2024 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants