-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
fix(editors): fix serialization/deserilization in editors #58
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,14 +91,22 @@ export class DateEditor implements Editor { | |
} | ||
|
||
serializeValue() { | ||
const domValue: string = this.$input.val(); | ||
|
||
if (!domValue) return ''; | ||
|
||
const outputFormat = mapMomentDateFormatWithFieldType(this.args.column.type || FieldType.dateIso); | ||
const value = moment(this.defaultDate).format(outputFormat); | ||
const value = moment(domValue).format(outputFormat); | ||
|
||
return value; | ||
} | ||
|
||
applyValue(item: any, state: any) { | ||
item[this.args.column.field] = state; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm is that necessary? I thought the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i guess it depends how you want to handle this. Currently There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah right I didn't think about that, it is a string, I guess your change is fine then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so do you think it is right to serialize the date as a string and then deserilize in
And i think a date is a simple object that can be passed around after the editor is destroyed right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot to mention that the Excel Copy Buffer uses a lot, if not all, of these functions from the Editors. If you want to see that it works correctly, you can enable More info about the Excel Copy Buffer If it all works with the Excel Copy then the PR is good There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can add a variable in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to make the Excel Copy Buffer Plugin working before we can say that the Editor works correctly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I got it to work by adding
Was there a reason you did not import flatpickr like UPDATE: the weird thing is the typescript compiler complains there is no default export for flatpickr, but there is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The I think I removed the delete of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for that clarification, I keep forgetting about the CLI/RequireJS. I pushed the commit that gets the date to work with the excel cell copy |
||
if (!state) return; | ||
|
||
const outputFormat = mapMomentDateFormatWithFieldType(this.args.column.type || FieldType.dateIso); | ||
|
||
item[this.args.column.field] = moment(state, outputFormat).toDate(); | ||
} | ||
|
||
isValueChanged() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -119,7 +119,8 @@ export class SingleSelectEditor implements Editor { | |
|
||
loadValue(item: any): void { | ||
// convert to string because that is how the DOM will return these values | ||
this.defaultValue = item[this.columnDef.field].toString(); | ||
// make sure the prop exists first | ||
this.defaultValue = item[this.columnDef.field] && item[this.columnDef.field].toString(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh I learned something new, I didn't know we could write it this way. I always write the long way of doing the check of the property then do a ternary operator |
||
|
||
this.$editorElm.find('option').each((i: number, $e: any) => { | ||
if (this.defaultValue === $e.value) { | ||
|
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.
why not just
const domValue: string = this.$input.val() || '';
?That is what I've done in my other repo
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
domValue
is anything other than a date (e.g. null, undefined or '') and you pass that to moment, the returned serialized value will be 'Invalid Date'