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

jmzagorski · 2018-05-16T02:12:49Z

dateEditor was serializing/deserializing wrong value. Also need to check for empty date value because moment will throw an error. singleSelect and multipleSelectEditor were serializing the actual value (e.g number type) when it should be serializing to a string and applyValue should be deserializing from string to actual type (e.g. number).

closes #56

dateEditor was serializing/deserializing wrong value. Also need to check for empty date value because moment will throw an error. `singleSelect` and `multipleSelectEditor` were serializing the actual value (e.g number type) when it should be serializing to a string and `applyValue` should be deserializing from string to actual type (e.g. number). closes #56

jmzagorski · 2018-05-16T02:26:48Z

@ghiscoding i was re-reading the serializedValue method description here again and I had once thought serializedValue should always be a string (because that is normally the result of serilization), but the description says it can be a simple arbitrary value. Do you think I should have left the serialized value the actual type from the original collection as opposed to returning the string value(s) from the $input? for the singleSelectEditor and multipleSelectEditor? For example, before if the valueName value on the item was 2 i serialized to 2 (same type), but with this PR i serialize to "2".

UPDATE: it is late for me and that change to singleSelectEditor and multipleSelectEditor is wrong lol. I should serialize to the actual type since it should always be a simple type (e.g. number, string, bool etc)

not sure why i thought I should always make the serialized value a string. Probabaly because I was chaning the `dateEditor` at the same time. Anyway, the serailied value from the `valueName` property should always be a simple type.

ghiscoding · 2018-05-16T03:24:45Z

aurelia-slickgrid/src/aurelia-slickgrid/editors/dateEditor.ts

@@ -91,14 +91,22 @@ export class DateEditor implements Editor {
  }

  serializeValue() {
+    const domValue: string = this.$input.val();


why not just const domValue: string = this.$input.val() || ''; ?
That is what I've done in my other repo

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'

ghiscoding · 2018-05-16T03:26:24Z

aurelia-slickgrid/src/aurelia-slickgrid/editors/dateEditor.ts


    return value;
  }

  applyValue(item: any, state: any) {
-    item[this.args.column.field] = state;


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

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;

ah right I didn't think about that, it is a string, I guess your change is fine then

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?

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

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;

We need to make the Excel Copy Buffer Plugin working before we can say that the Editor works correctly

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:

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.

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.

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.

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

ghiscoding · 2018-05-16T03:29:30Z

aurelia-slickgrid/src/aurelia-slickgrid/editors/singleSelectEditor.ts

@@ -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();


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

ghiscoding · 2018-05-16T13:12:31Z

I think your code looks good, you can merge whenever you are ready. I'm not sure if you were done with it or not.

jmzagorski · 2018-05-16T13:14:39Z

I am. I just wanted to make sure you were okay with my responses to your comments and if they make sense?

ghiscoding · 2018-05-16T13:16:33Z

yeah it looks ok

jmzagorski · 2018-05-16T14:11:37Z

I added one more comment on the dateEditor to get your opinion

ghiscoding · 2018-05-16T14:22:27Z

I forgot about the Excel Copy Buffer Plugin, I should have thought about that earlier, but anyway that will be the best way to test the Editors, See the note on top

ghiscoding · 2018-05-17T18:00:54Z

@jmzagorski
added a comment to your question on top

this will allow the excel cell copy to work when serializing this.$input.value()

jmzagorski · 2018-05-17T20:03:14Z

@ghiscoding off topic, but i didn't know where to say this. If you pull down the latest master and run your demo, are the inline editors working in example 3? All of them but the longtext and date are not working for me.

jmzagorski · 2018-05-17T20:10:17Z

Oh yeah i meant to tell you this, if you enableCheckboxSelector, some inline editors stop working. I noticed this in my application, but I removed the checkboxselector and forgot about this problem. In example 3 if you comment out enableCheckboxSelect: true in gridOptions, the editors work again

ghiscoding · 2018-05-17T20:59:03Z

@jmzagorski
Probably related to SlickGrid issue #68

I had a PR #241 that got merged in last SlickGrid version 2.3.18 but I'm not sure if it will be reverted or not, it's causing some issue #242

Anyhow, you need version 2.3.18 for it to work, if you don't then Row Selection and Editors aren't working properly together. The version upgrade is in master now, but you might not have it in this branch and/or you didn't upgrade your local

jmzagorski · 2018-05-17T21:13:40Z

ahh i had a feeling i had something out of date. I will check that out later tonight or tomorrow

As for this PR, it should be ready.

ghiscoding · 2018-05-18T13:14:12Z

Are you done with the PR? If so, please merge and close the issue. I find there is too many open issue, not crazy about it, it looks like the lib is not yet stable.

Also note, there is yet a newer version of SlickGrid 2.3.19 with a new flag to partially reverse a PR I made in previous version. I will update the version in a later commit

ghiscoding · 2018-05-18T15:50:14Z

@jmzagorski
Another side note to the (Row Selection + Inline Editors) used at same time. I found out that you cannot use the Excel Copy Buffer plugin at the same time. Because the row selection and excel copy both use the Slick.CellSelectionModel and one cancels the other. I added a note in the Wiki - Row Selection, let me know if that is not enough in the docs... oh that is more of a SlickGrid issue, not our lib, so I don't think we need a ticket for that, hopefully a note is enough

jmzagorski · 2018-05-18T16:47:34Z

👍 looks good, thank you.

ghiscoding · 2018-05-19T18:02:24Z

FYI, SlickGrid is now at version 2.3.19 and was updated on our side in this commit

jmzagorski requested a review from ghiscoding May 16, 2018 02:13

ghiscoding reviewed May 16, 2018

View reviewed changes

ghiscoding added this to To do in v1.x May 16, 2018

ghiscoding moved this from To do to In progress / Ready to Merge in v1.x May 16, 2018

refactor(editors): load defaultDate into flatpickr on loadValue

7bc8baf

this will allow the excel cell copy to work when serializing this.$input.value()

jmzagorski merged commit f3610d7 into master May 18, 2018

jmzagorski deleted the fix/editor_serialization branch May 18, 2018 13:21

ghiscoding moved this from In progress / Ready to Merge to Done / Shipped / Merged in v1.x May 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jmzagorski commented May 16, 2018

jmzagorski commented May 16, 2018 •

edited

ghiscoding May 16, 2018

jmzagorski May 16, 2018

ghiscoding May 16, 2018

jmzagorski May 16, 2018

ghiscoding May 16, 2018

jmzagorski May 16, 2018

ghiscoding May 16, 2018

ghiscoding May 16, 2018

ghiscoding May 16, 2018

jmzagorski May 17, 2018 •

edited

ghiscoding May 17, 2018

jmzagorski May 17, 2018

ghiscoding May 16, 2018

ghiscoding commented May 16, 2018

jmzagorski commented May 16, 2018

ghiscoding commented May 16, 2018

jmzagorski commented May 16, 2018

ghiscoding commented May 16, 2018

ghiscoding commented May 17, 2018

jmzagorski commented May 17, 2018

jmzagorski commented May 17, 2018

ghiscoding commented May 17, 2018

jmzagorski commented May 17, 2018

ghiscoding commented May 18, 2018 •

edited

ghiscoding commented May 18, 2018 •

edited

jmzagorski commented May 18, 2018

ghiscoding commented May 19, 2018

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

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

Conversation

jmzagorski commented May 16, 2018

jmzagorski commented May 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmzagorski May 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghiscoding commented May 16, 2018

jmzagorski commented May 16, 2018

ghiscoding commented May 16, 2018

jmzagorski commented May 16, 2018

ghiscoding commented May 16, 2018

ghiscoding commented May 17, 2018

jmzagorski commented May 17, 2018

jmzagorski commented May 17, 2018

ghiscoding commented May 17, 2018

jmzagorski commented May 17, 2018

ghiscoding commented May 18, 2018 • edited

ghiscoding commented May 18, 2018 • edited

jmzagorski commented May 18, 2018

ghiscoding commented May 19, 2018

jmzagorski commented May 16, 2018 •

edited

jmzagorski May 17, 2018 •

edited

ghiscoding commented May 18, 2018 •

edited

ghiscoding commented May 18, 2018 •

edited