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
Feature/data picker #1779
Feature/data picker #1779
Conversation
Hey Will, haven't had time to fully review yet, few questions first:
|
|
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.
Great work! It'll be cool to have this in the app.
Questions:
- vue2-datepicker looks abandoned. Are there any pure-js alternatives?
- Let's invert how we do the component - so we don't materially change the
NullableInputEditor
.
@@ -1,6 +1,20 @@ | |||
<template> | |||
<div> | |||
<date-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.
I don't think we want to pollute the NullableInputEditor
with the date picker.
Can we do this in a different way:
- Add a named slot to the
NullableInputEditor
for the custom button - Pass in the calendar icon button into the slot
- Then when that button is clicked it can show the date modal, this can then live a whole level UP from the nullableinputeditor.
Something like this: DatePickerEditor.vue
<NullableInputEditor ...>
<template v-slot:buttons>
<DatePicker ... />
</template>
</NullableInputEditor
return '' | ||
}, | ||
typeEditorIcon() { | ||
if (this.typeEditorActive) return 'edit' |
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.
Yep, then all this stuff can move out of the NullableInputEditor and into a DatePickerEditor
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.
Yeah that was all part of the plan for a later PR. Was taking this approach as a get this out there and see how people like it first. I'll make the changes
@@ -15994,6 +15999,13 @@ vue-template-es2015-compiler@^1.6.0, vue-template-es2015-compiler@^1.9.0: | |||
resolved "https://registry.yarnpkg.com/vue-template-es2015-compiler/-/vue-template-es2015-compiler-1.9.1.tgz#1ee3bc9a16ecbf5118be334bb15f9c46f82f5825" | |||
integrity sha512-4gDntzrifFnCEvyoO8PqyJDmguXgVPxKiIxrBKjIowvL9l+N66196+72XVYR8BBf1Uv1Fgt3bGevJ+sEmxfZzw== | |||
|
|||
vue2-datepicker@^3.11.1: |
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.
This looks pretty abandoned.... plus it will break with vue3. Are there any javascript-only implementations?
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 can look for a pure JS version, was hoping to use a vue specific one but can look into another one. Had a couple people recommend this one to me.
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.
Have you had a look at Vuetify's date pickers?
Might be nice, and you should be able to only use the date pickers, so it wont mess up the bundle size.
$namespace: 'mx' !default; | ||
|
||
$default-color: #73879c !default; | ||
$primary-color: #1284e7 !default; |
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.
Does this all work in light mode, solarized, and solarized dark?
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 tried it in light and dark mode and it looked pretty good.
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.
Def should not be re-implementing the nullableinputeditor, let's just add a slot for the button!
confirm-text="click to confirm new selection" | ||
@confirm="submit" | ||
/> | ||
<input |
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.
So we don't want to re-implement the whole nullable editor inside the DateTimePickerEditor.
Have you used Vue's slots before? We should add a slot to NullableInput
, like this:
<div>
<input
class="nullible-input"
:placeholder="smartPlaceholder"
ref="input"
type="text"
v-model="value"
@blur.prevent="submit"
@change.prevent="submit"
@keydown="keydown"
>
<span class="nullable-input-buttons">
<slot name="buttons" />
<i
class="material-icons clear"
@mousedown.prevent.stop="clear"
title="Nullify Value"
>cancel</i>
</span>
</div>
Then your datetime picker can look like this:
<template>
<NullableInputEditor>
<template v-slot:buttons>
<i class="material-icons" @click.prevent="pickDate">calendar</i>
</template>
</NullableInputEditor>
</template>
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.
Slot has been added to handle this. Should be decently extendable.
… into feature/data-picker
TODO: null button not working, get the value from the original editor
@wmontgomery You'll need to merge master into this |
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.
Looks good overall!
Had a couple of minor issues, please look at before merging!
|
||
if (this.isTimeType(dataType) && dataValue !== null) { | ||
dataValue = dataValue.search(/(\+|-)/i) > -1 && !isNaN(dataValue.slice(-1)) ? `${dataValue}:00`: dataValue | ||
this.datePickerValue = new Date(`2023-03-31T${dataValue}`) |
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.
Where did this date come from? Should this be today's date?
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.
If not, or it doesn't matter, please add a comment or something
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.
It shouldn't matter what the date is since we are really just creating a date object to grab the time, it was a Parks and Recreations reference. There are no meetings on Mar 31st because it's not a real day.
this.$emit('value', null) | ||
} | ||
}, | ||
watch: { | ||
rendered() { | ||
if (this.rendered) { | ||
this.value = helpers.niceString(this.cell.getValue()) | ||
this.value = this.cell.getValue() == null ? null : helpers.niceString(this.cell.getValue()) |
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.
Maybe use _.isNil(this.cell.getValue())
@@ -50,6 +50,9 @@ export default { | |||
const result = `<pre>${cellValue}</pre>` | |||
return result; | |||
}, | |||
isDateTime(dataType: string|null) { |
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 this is fine for now, but I think we probably need to add bksType
to our ColumnDefinition
interface at some point to identify stuff like this at the driver level, rather than in the GUI
#1505