Custom DateTime Selector #790
Conversation
for more information, see https://pre-commit.ci
|
@garrettmflynn Could you rebase this one while I continue work on the Flask side? |
for more information, see https://pre-commit.ci
…taWithoutBorders/nwb-guide into custom-datetime-selector
for more information, see https://pre-commit.ci
|
@garrettmflynn This biggest issue we definitely need to fix is the exposure of a 'timezone' field like the one seen on the google docs (the one that auto-populates from making a 'meeting notes' template) I see the role of the new 'limited' range selector would be to prevent the selection of far-future values; but how would we automatically set such an upper bound?
|
|
Yeah I'm currently trusting system time and assuming you can't add a future date. I imagine there's an API for getting "absolute" time using a network request. Maybe we use that? |
|
I like that idea a lot! Of course it would only work in 'online' mode, so I guess trust system time only as a fallback? Did you want to implement that in this PR? |
|
I'd do this separately, as the intention of this PR is to create a new component with an optional time / more time specificity. The date limits should already be active, I'm pretty sure I've just simplified the behavior here. If that isn't the case, then we may want to get that in with the absolute time adjustment. |
|
@CodyCBakerPhD Did you intend to merge this with Just want to make sure we're aligned on our approach for this feature. We originally discussed keeping a PR open as a placeholder for the component so it would be clear where to implement / test it. |
|
@garrettmflynn Yeah, to my eyes this PR added the min/max properties to the current selector, and you said we'd add the setting of those values in a follow-up? |
|
Gotcha. Those properties were already configurable on the native DateTime input within the component, I'd just exposed the As mentioned above, this feature should have been active in So aside from the Story, nothing had fundamentally changed with this PR. We can still add the "absolute time" through an API call in a follow-up—though as per the original comment suggesting this PR, there may still be cause to reopen a PR like this to ensure a custom DateTime component with improved time selectivity is eventually completed. |
|
Hey guys, what’s the status of timezone control in GUIDE? I searched and found these lines: nwb-guide/src/schemas/base-metadata.schema.ts Lines 45 to 49 in 224675f but I’m not sure what’s going on here. Are we taking the local timezone? Do we have a way for the user to override this? If we don't currently have this, a simple solution might be for the user to select a timezone on the initial conversion configuration page and then automatically apply this timezone to all datetimes. This would have the limitation that it would be impossible for user to specify multiple datetimes in the same conversion with different timezones, but I think that would be a fairly rare scenario. |
|
As mentioned in #790 (comment) this is the priority of the next follow-up Current behavior is same as PyNWB or default NeuroConv (using system timezone) I'd say this takes priority over #732 for the v1 release @garrettmflynn |
|
OK, thanks for the clarification, I wasn't sure what you meant by Google Docs. @garrettmflynn to fill you in, we've had a bit of a situation with timezones. We tried to make them optional in PyNWB and that caused subtle validation issues on the DANDI server side which is now preventing NWB files from being publishable. We are changing PyNWB timezones back to being a requirement. The issue is that I am expecting to have lots of GUIDE users that are using GUIDE at a workshop that is far from their home lab and therefore their local timezone will not be the correct time of the session start time. That is the context for why timezone control has all of a sudden become more important. I recognize that this is a tricky UI component given the table-like structure we are using for the subjects table page. That's why I'm trying to propose a solution that would allow for timezone control via a separate UI component that might be easier to implement. |
|
Screenshot of google docs component: #591 (comment) |
|
ok thanks so with the new change I don't know if we want to make the timezone optional. We could try to detect the system timezone and select that be default. |
|
Yep I think I see a path forward. I'll get a quick PR out :) |
|
@CodyCBakerPhD Just so I get this right, what is the expected input to PyNWB? ISO DateTime + timezone in "America/NewYork" format? |
|
Also just to emphasize, the current snippet that Ben highlighted does already limit the user's selection of the timestamp to the appropriate value in the current timezone (or at least |
|
https://docs.python.org/3/library/zoneinfo.html#zoneinfo.available_timezones {'Africa/Abidjan', |
PyNWB expects only a The easiest way to set all that is as seen throughout the NeuroConv gallery, such as: https://neuroconv.readthedocs.io/en/main/conversion_examples_gallery/recording/biocam.html
What I would do is just have a dropdown from +0 to +/- 12 with the 'most popular/likely' ZoneInfo abbreviation attached to it, for example |
|
I was imagining a dropdown that also allows you to type in and filter. Having a really long dropdown is not the best, but it's also not terrible. Cody, your idea of a pared down set with the offset would work except that the offset changes depending on daylight savings. zoneinfo handles this automatically, but due to DST, we would need to know the day before we know the offset. |
|
Since we started this conversation, I've been able to render both a strict Just indicating this so y'all know we're not constrained on the frontend—just, it seems, in terms of how Python will ingest the datetime information and convert to a |
This PR addresses #591, specifically starting a draft PR that shows how to get started with a fix.
In particular, it creates a relevant Storybook story to highlight the existing
DateTimeSelectorcomponent that can be modified to replace the native date-time popup with an element that optionally includes time.