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

Timezone Control in the GUIDE #820

Merged
merged 61 commits into from
Jun 5, 2024
Merged

Timezone Control in the GUIDE #820

merged 61 commits into from
Jun 5, 2024

Conversation

garrettmflynn
Copy link
Member

@garrettmflynn garrettmflynn commented Jun 3, 2024

This PR allows users to control the timezone used across the GUIDE from the Workflow page. Currently, this PR only allows you to select a timezone—not to actually apply this timezone to the resulting data.

@CodyCBakerPhD Still not 100% sure what the best way to propagate the timezone to the appropriate places in the backend? Would you be able to indicate how I should pass a timezone to NWB Inspector (e.g. using the validate_nwbfile_metadata function) as well as to NeuroConv during the conversion process?

This PR originated from a discussion following the merge of #790.

@garrettmflynn garrettmflynn self-assigned this Jun 3, 2024
Comment on lines 4 to 26
const mostCommonTimezonesWithUTCOffset = {
'Pacific/Honolulu': '-10:00',
'America/Anchorage': '-09:00',
'America/Los_Angeles': '-08:00',
'America/Denver': '-07:00',
'America/Chicago': '-06:00',
'America/New_York': '-05:00',
'America/Sao_Paulo': '-03:00',
'Atlantic/Azores': '-01:00',
'Europe/London': '+00:00',
'Europe/Paris': '+01:00',
'Europe/Athens': '+02:00',
'Asia/Jerusalem': '+02:00',
'Europe/Moscow': '+03:00',
'Asia/Dubai': '+04:00',
'Asia/Karachi': '+05:00',
'Asia/Dhaka': '+06:00',
'Asia/Jakarta': '+07:00',
'Asia/Shanghai': '+08:00',
'Asia/Tokyo': '+09:00',
'Australia/Sydney': '+10:00',
'Pacific/Auckland': '+12:00'
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is likely to confuse people due to daylight savings time. Most of these timezone names actually have different offsets depending on the day of the year. zoneinfo takes care of this automatically- you input the name of the timezone and it will check the date so that the correct offset is applied. I wish we could apply a simplifying approach like this but honestly I think this is going to confuse a lot of people if implemented this way

Copy link
Collaborator

Choose a reason for hiding this comment

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

I honestly think a very long dropdown would be better. I know it's not ideal but it is at least accurate

Copy link
Member Author

Choose a reason for hiding this comment

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

Just pushed the option where we are rendering the browser's acceptable timezones as a native <select> element.

Screenshot 2024-06-03 at 12 21 00 PM

How's this? Would you prefer it to be searchable? What about the formats used by zoneinfo that aren't in America/Chicago format?

Copy link
Collaborator

@CodyCBakerPhD CodyCBakerPhD Jun 3, 2024

Choose a reason for hiding this comment

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

This approach would absolutely need to be searchable (probably as our loose selector); it's overwhelming scrolling through a list that massive

I'd still recommend we simplify with a basic numeric scroller, that's many fewer options and removes the complexity of tying the number to a specific place (which has a somewhat low chance of being exactly the city they are located in)

@bendichter
Copy link
Collaborator

I guess a simple alternative would be to just provide offsets without names

@CodyCBakerPhD
Copy link
Collaborator

CodyCBakerPhD commented Jun 3, 2024

I guess a simple alternative would be to just provide offsets without names

I think this might be best overall

I really worry about

I honestly think a very long dropdown would be better. I know it's not ideal but it is at least accurate

Because we already get feedback in user tests about information overload

And basic google search of 'what time zone am i in' (maybe we can have a description hyperlink find my timezone somewhere?) reports the offset for GMT, so I don't think it's unreasonable for users to be responsible for looking up their own timezone if they don't know it

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Jun 3, 2024

@CodyCBakerPhD Can you take a look at this? Almost ready, just think we need to adjust NWB Inspector to allow for adjusted timestamps. At present, a timestamp that is correctly rejected will not have a reaction when the GMT offset is added.

@CodyCBakerPhD
Copy link
Collaborator

I don't see a timezone area on the session start time selector for the file metadata page

image

Was there something else you wanted me to look at?

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Jun 4, 2024

Oh sorry, this just implements a global configurable timezone on the Workflow page.

Implementing a field-level selector will be significantly more complicated, if only for the back-and-forth we'll need to finalize the UI. Should squeeze that in with an optional time—as those UI components are going to have to play nice together.

@rly
Copy link
Collaborator

rly commented Jun 4, 2024

Could we make the user's current time zone appear first in the list? That's the most common case.

Secondly, could we enable search on any part of the time zone name? E.g. currently searching "new" does not return anything. I have to start with "America/New"

@rly
Copy link
Collaborator

rly commented Jun 4, 2024

I found some interesting ux data on time zone selectors: https://www.nngroup.com/articles/time-zone-selectors/
(TL;DR guide at the bottom)

@CodyCBakerPhD
Copy link
Collaborator

Oh sorry, this just implements a global configurable timezone on the Workflow page.

Ah, thanks I missed that

If I click off of the selector mid-choice it autofills with this

image

and I have to delete it to get back to valid choices

I can also proceed to the next pages with undefined but if I go back it reverts to Pacific/Tongatapu for some reason

I would also make this a required field (red asterisk)

@CodyCBakerPhD
Copy link
Collaborator

Also when I try to create a non-stub file I get this error displayed

image

But no console.log or log file contains a traceback - is this something on the frontend?

@CodyCBakerPhD
Copy link
Collaborator

@garrettmflynn This should be working now, can you give it a whirl?

One thing is I may have broken that nice section rendering of the zoneinfo loose selector (by 'America' and so forth) but it at least seems functional

@garrettmflynn
Copy link
Member Author

Looks good! That change is on me, I've added it back now that we're pulling from the Python list.

Screenshot 2024-06-04 at 4 37 17 PM

@CodyCBakerPhD
Copy link
Collaborator

Oky doky, last thing is just fixing the tests then this is good to go

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge June 5, 2024 01:54
@CodyCBakerPhD
Copy link
Collaborator

@garrettmflynn According to chromatic, this PR also introduces the 'edit default values' button to all source data pages'?

There also seem to be 3 missing story pages: https://www.chromatic.com/build?appId=640a4d93e34604f275368983&number=997

Any idea what that's about?

@garrettmflynn
Copy link
Member Author

Yes working on it now. Seems to be something we didn't catch during the refactor after trying to remove the dev (web) mode.

The stories rely on storyStates.ts and saving / loading progress files—which was, in large part, removed.

Some of this was that I wasn't providing a timezone properly without backend access. But there's more to the story than that.

@garrettmflynn
Copy link
Member Author

Should be fixed. A lot of this also had to do with the addition of the Workflow (Preform) page.

@CodyCBakerPhD
Copy link
Collaborator

Great, thanks!

@CodyCBakerPhD CodyCBakerPhD merged commit 47467bb into main Jun 5, 2024
21 of 22 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the timezone branch June 5, 2024 15:25
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.

4 participants