Conversation
🦋 Changeset detectedLatest commit: 9554fac The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| import { Temporal } from 'temporal-polyfill'; | ||
| import { type CodecDecoder, type CodecEncoder, ValueCodec } from './ValueCodec.ts'; | ||
|
|
||
| export type TimeRuntimeValue = string | null; |
There was a problem hiding this comment.
Temporal doesn't have a type that represents only time with an offset. PlainTime is time only.
I think string is okay for now.
| export type TimeRuntimeValue = string | null; | ||
|
|
||
| export type TimeInputValue = | ||
| | Date |
There was a problem hiding this comment.
PrimeVue uses Date.
|
@garethbowen this is ready for review :) |
| return new Date(`${yyyy}-${mm}-${dd}T${temporalValue}`); | ||
| }, | ||
| set: (newTime) => { | ||
| // Clear seconds and milliseconds to match Collect and Enketo behavior (a client's responsibility). |
There was a problem hiding this comment.
Enketo and Collect behavior:
- If the question has a predefined value with seconds and milliseconds, e.g.,
22:02:10.562+06:00, and it wasn't modified, the seconds and milliseconds are preserved. - If the question has a predefined value with seconds and milliseconds, and the value was modified, then seconds and milliseconds are zero.
- If no predefined value exists and a new one is set, then seconds and milliseconds are zero.
- All values captured using the time picker are saved in the format
HH:mm:ss.SSSXXX, e.g.,22:02:10.562+06:00
garethbowen
left a comment
garethbowen
left a comment
There was a problem hiding this comment.
Nice work! Some minor suggestions inline...
Times having timezones broke my brain a little. I think it's more correct that they don't but the XML spec isn't clear: https://www.w3.org/TR/xmlschema-2/#time
Regardless I'm comfortable with the inclusion of timezone until proven wrong!
| const today = new Date(); | ||
| const yyyy = today.getFullYear(); | ||
| const mm = String(today.getMonth() + 1).padStart(2, '0'); | ||
| const dd = String(today.getDate()).padStart(2, '0'); | ||
| return new Date(`${yyyy}-${mm}-${dd}T${temporalValue}`); |
There was a problem hiding this comment.
We have the Temporal plugin in web-forms, right?
If so, I think this can be replaced with...
| const today = new Date(); | |
| const yyyy = today.getFullYear(); | |
| const mm = String(today.getMonth() + 1).padStart(2, '0'); | |
| const dd = String(today.getDate()).padStart(2, '0'); | |
| return new Date(`${yyyy}-${mm}-${dd}T${temporalValue}`); | |
| const today = Temporal.Now.plainDateISO(); | |
| return new Date(`${today}T${temporalValue}`); |
There was a problem hiding this comment.
No, we only use Temporal in xpath, xforms-engine, and scenario right now. Perhaps we can hold off until we see more usage before including it. What do you think?
There was a problem hiding this comment.
Ok leave it for now. Ideally it would be a peer dependency in the xforms-engine too so it wouldn't get bundled and then it would be "free" to use in all packages. Anyway, stick with this for now.
| </script> | ||
|
|
||
| <template> | ||
| <DatePicker v-model="value" time-only hour-format="12" :disabled="isDisabled" /> |
There was a problem hiding this comment.
It would be nice to showIcon here like we do with the date picker, though ideally it would be a clock icon not a calendar. Not sure how hard that is to do...
I was thinking the same! But then the ODK spec says this:
I've resolved the feedback :) |
garethbowen
left a comment
garethbowen
left a comment
There was a problem hiding this comment.
Beautiful!
Don't forget to update the feature matrix while you're at it!
2 participants
Closes #590
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
Tested in Firefox, Chrome, Safari, and Chrome for Android
Tested editing submission in Central
Why is this the best possible solution? Were any other approaches considered?
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Do we need any specific form for testing your changes? If so, please attach one.
Test server available in team's channel
What's changed