Skip to content
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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 18, 2018
Merged

fix(editors): fix serialization/deserilization in editors #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ee03b0
fix(editors): fix serialization/deserilization in editors
jmzagorski May 16, 2018
016f070
refactor(editors): serialize select editors to actual type
jmzagorski May 16, 2018
7bc8baf
refactor(editors): load defaultDate into flatpickr on loadValue
jmzagorski May 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions aurelia-slickgrid/src/aurelia-slickgrid/editors/dateEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,17 +88,26 @@ export class DateEditor implements Editor {

loadValue(item: any) {
this.defaultDate = item[this.args.column.field];
this.flatInstance.setDate(item[this.args.column.field]);
}

serializeValue() {
const domValue: string = this.$input.val();
Copy link
Owner

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

Copy link
Collaborator Author

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'


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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm is that necessary? I thought the state was already formatted by the serializer

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess it depends how you want to handle this. Currently serailizedValue method returns a string date using moment(date).format(string). If you used just the state value in applyValue then the original property for this editor will go from a date object type to a string type, and I do not think you want that? If you do not want to serialize it as a string, then this logic can move into the serializedValue method so the return value is a date object and then applyValue will go back to item[this.args.column.field] = state;

Copy link
Owner

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 applyValue. The only downside I can see with that is that if someone overrides the editCommandHandler, the serializedValue will be a string and not a date object. Per the slickgrid documents he describes the function of serializedValue as:

// return the value(s) being edited by the user in a serialized form
// can be an arbitrary object
// the only restriction is that it must be a simple object that can be passed around even
// when the editor itself has been destroyed

And i think a date is a simple object that can be passed around after the editor is destroyed right?

Copy link
Owner

Choose a reason for hiding this comment

The 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 enableExcelCopyBuffer: true in the grid options and copy multiple cells with the mouse then try to paste it in Excel. If there's anything wrong with the Editors, you will see it throw some errors during the copy.

More info about the Excel Copy Buffer

If it all works with the Excel Copy then the PR is good

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add a variable in the onChange to keep actual date and if never changed then just return the defaultDate. So something like this
return this.currentDate || this.defaultDate;

Copy link
Owner

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

@jmzagorski jmzagorski May 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it to work by adding this.flatInstance.setDate(item[this.args.column.field]) in loadValue right after you set the defaultDate. However, I had a few questions before i commit my changes:

  1. Is there a reason why you do not destroy flatpickr? This was commented out and instead you close it. However, it seems on init you are resetting the instance with a new element anyway.
  2. You check for flatpickr with this code:

this.flatInstance = (flatpickr && this.$input[0] && typeof this.$input[0].flatpickr === 'function') ? this.$input[0].flatpickr(pickerOptions) : null;

Was there a reason you did not import flatpickr like import flatpickr from 'flatpickr and then initialize it like this.flatInstance = flatpickr ? flatpickr(this.$input.get(0), pickerOptions) : null;. In their documents it says this is the preferred way if their library is in a framework.

UPDATE: the weird thing is the typescript compiler complains there is no default export for flatpickr, but there is.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flatpick library is not that great with how to import it in TS, they changed it a few times too in trying to make it work in TS. Also it was the only way I got it working with both WebPack and RequireJS (the client-cli demo I have). So please don't change the import, it works now, don't break it again lol

I think I removed the delete of the flatInstance because SlickGrid destroys the DOM element even before flatInstance. Might not be exactly that, but it was something near that behavior.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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() {
Expand Down
3 changes: 2 additions & 1 deletion aurelia-slickgrid/src/aurelia-slickgrid/editors/singleSelectEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Owner

Choose a reason for hiding this comment

The 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) {
Expand Down