-
Notifications
You must be signed in to change notification settings - Fork 2
Clock UI #282
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
base: main
Are you sure you want to change the base?
Clock UI #282
Conversation
chrimyn234
commented
Jun 6, 2024
- Implement a Google calendar inspired dropdown style time picker.
- Add new timepickerType field to meal time prompt to select between 'clock' and 'simple' .
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.
Hi Chris, thanks for the PR!
I've added couple of comments what I would improve / add.
Can you please also add newly added options to the docs (to the admin / prompt types section)? Thanks!
z.literal(20), | ||
z.literal(30), | ||
]), | ||
timepickerType: z.enum(['clock', 'simple']), |
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.
timePickerType
is bit repetitive, could we call it e.g.style
?- could make the
simple
more descriptive, something likedropdown
to hit what the style is? ... if we have few more styles in future, simple might quickly collide.
const timePickerPrompt = baseCustomPrompt.merge(promptValidationProps).merge(timePicker).extend({ | ||
component: z.literal('time-picker-prompt'), | ||
}); | ||
const timePickerPrompt = baseCustomPrompt |
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.
survey frontend timepicker implementation is missing, only meal-time-prompt
is in PR, can you please add?
@@ -0,0 +1,116 @@ | |||
<template> | |||
<div> |
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.
is the <div>
wrapper necessary?
@@ -0,0 +1,116 @@ | |||
<template> | |||
<div> | |||
<v-combobox |
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 think combox also allows to enter values manually / typed-in, if it's intention, you willl need to validate output so the typed-in time format is correct. If only select is intended, simple <v-select>
can be used
type: Number, | ||
required: true, | ||
}, | ||
is12HourFormat: { |
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.
use same format as already defined in prompt props -> ampm | 24hr
, so can be used.
<v-card-text class="pt-2 meal-time-prompt"> | ||
<v-form @submit.prevent="action('next')"> | ||
<time-picker |
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.
please use dynamic component instead of switch
components can be registered as
components: {clock: 'v-time-picker', simple: 'time-picker'}
and then used as
<component :is="prompt.style" v-model="state" :allowed-minutes="allowedMinutes" :format="prompt.format">
@@ -93,6 +93,7 @@ export const timePickerPrompt: Prompts['time-picker-prompt'] = copy({ | |||
type: 'custom', | |||
id: 'time-picker-prompt', | |||
name: 'Time picker prompt', | |||
timepickerType: 'clock', |
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.
can you please add survey frontend impl for the custom time picker?
pnpm-lock.yaml
Outdated
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 think changes to the lock can be dropped altogether as dependencies are not touched, probably just a rebase artifact
a76af83
to
bba98c5
Compare