feat(selectEditors): add select grid editors #22

jmzagorski · 2018-03-03T18:54:03Z

This pull request brings two new editors and one small change and should close #18

Purpose
Select editors are very common with data to ensure data integrity and we already provide these are header filters.

Features

SelectListEditor - this editor uses most of the logic from the SelectListFilter. If the developer does not use multiple-select library, this editor falls back to a bootstrap form-control. For this to render property in the grid a new css rules was added.
MultipleSelectListEditor - this editor uses most of the logic from the MultipleSelectListFilter. If the developer does not use multiple-select library, this editor falls back to a bootstrap form-control
CollectionFormatter - the formatter will lookup the label value in the filter.collection based on the value passed into the formatter. If it is an array, it will use the arrayToCsvFormatter
arraysEqual- a utility function to compare all values in an array for the MultipleSelectListEditor
Array.prototype.findOrDefault - An extension function to make code look cleaner especially when finding complex objects. If find returns undefined the consumer of this function can still safely try and access the property since by default the returned value is an empty object.

Changes

The arrayToCsvFormatter was changed to a span with a title in cases where the csv string is too long to show all values

Tests
I didn't start creating unit tests yet because aurelia-pal-nodejs needs to be updated because of this fix aurelia/pal-nodejs#24 and another error i need to research about imports. Therefore, I manually tested with example 3 by changing the editor on the effort-driven column and adding/removing the multiple-select lib.

Comments

(Future) We could probably improve the logic this.labelName = (this.columnDef.filter.customStructure) ? this.columnDef.filter.customStructure.label : 'label'; since it is referenced in 3 files now. We could have a default columnDef.filter object that ensures the customStructure is always there.
(Future) We could refactor the logic to create the select DOM element since it is the same in the Filter and Editor or just wait to see if we can/want to suppose templates

Update WIKI

select editors are common with data to ensure data integrity

ghiscoding · 2018-03-03T19:36:23Z

Could you please modify 1 of the example, possibly Example 3, to include that editor?
Changing the "% complete" with that editor could be a good demo.

It would be easier to test it out.

EDIT
I looked at the code quickly, seems good maybe 1 thing though, I'm not sure if it's needed in an Editor but for the Filter and presets, I needed to add a setValues() function like multipleSelect - setValues() because the Filter always need to re-render after something changed in the grid like change a column position. All that to say, do we need that in an Editor? Maybe not

jmzagorski · 2018-03-03T21:49:22Z

added the SingleSelectEditor to the % complete and MultipleSelectEditor to a new Prerequisites column (it is the only new column I could think of that would have multiple values).

From the looks of it, I do not think the setValues needs to be in these editors after something changes in the grid since the editor is only active when a user interacts with the cell. You should be able to move a column and the editor should work the same. I have no experience yet with presets so you might want to test that example.

ghiscoding · 2018-03-03T21:54:00Z

I added the setValues (only in Angular for now) because if I move a column (drag and change column position), the filter loses it's value because SlickGrid re-renders after moving a column. Have you tried changing a columns position while the editor is open? if the editor closes then no problem I guess. I'm in the middle of implementing the "Grid State & Grid Presets", so I can't really test it right now.

Actually thinking about it while writing, I think the editor goes away while you drag a column since it goes away onBlur, is that right? if so, no need for that setValues. The setValues is coming in my current feature.

jmzagorski · 2018-03-03T22:14:37Z

It looks like the grid commits the change and closes the editor when moving a column while an editor is opened except for the new MultipleSelectEditor i just implemented. It behaves just like the other editors, but the select dialog box stays open.

ghiscoding · 2018-03-05T01:30:43Z

I'm practically done with my PR #23 but I'd like to get this PR here in place before pushing a new release. I have left some comments which we can discuss

jmzagorski · 2018-03-05T02:42:21Z

which comments did you want to discuss? Sorry I might have missed some

ghiscoding · 2018-03-05T02:51:05Z

see the review comments on top

jmzagorski · 2018-03-05T03:36:56Z

sorry I am looking for these "review comments" at the top and can't find them. I am probably looking in the wrong place and they are probably right in front of me somewhere. Once I find where those are I can review those tomorrow.

ghiscoding · 2018-03-03T19:45:51Z

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

+
+  init() {
+    if (!this.args) {
+      throw new Error('[Aurelia-SlickGrid] A filter must always have an "init()" ' +


any particular reason why you've put this on 2 lines?
Are you following the editor max length? I would rather see only 1 line here

I can change this to one line. I have a max line length of 80 in my editor. Do you have this setting in a file, if so what is it?

I don't have this setting and I don't follow it either. I understand it's for readability and I try to split into multiple lines when it's long but it's my own judgement

ghiscoding · 2018-03-05T00:07:52Z

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

+        '{ filter: type: FilterType.multipleSelect, collection: [{ value: true, label: \'True\' }, ' +
+        '{ value: false, label: \'False\'}] }');
+    }
+    this.optionCollection = this.columnDef.filter.collection || [];


looking at this, I don't really like seeing a filter object inside an editor. I would still rather see params.collection than filter.collection. Unless you have good reasons, I would rather change this to be more generic if possible, or put a generic property in the columnDef. Second note, I previously changed selectOptions to collection to be more generic, so the choice of optionCollection is rather similar, it's probably better than selectOptions anyhow.

The datalist actually prove that it was a good decision since selectOptions doesn't fit in. You could argue that optionCollection makes more sense and you would probably be right but since I've already changed to collection in the Filter, we should probably stick with that for now, until version 2.x (which I will add a note in the 2.x refactoring)

I can change it to reference params.collection. That should be no problem.

So do you want me to change the optionCollection property name in the editor to be named collection? optionCollection should be a private property in the editor since it is just a reference to collection, and to be honest I cannot remember why I called the property optionCollection

What about accessing the customStructure? I use this in both the editors and the collectionFormatter? Should that be params.customStructure too?

And the reason I chose to use the filter object is because i assumed most of the time the filter.collection and filter.customStructure would be the same for the editors collection and customStructure

To make it to work in both Editor and Filter, we will have to review all the Filters, for example the MultipleFilter should use this instead

-if (!this.columnDef || !this.columnDef.filter || !this.columnDef.filter.collection) { +if (!this.columnDef || !this.columnDef.filter || !this.columnDef.filter.collection || this.columnDef.params.collection) { throw new Error(`[Aurelia-SlickGrid] You need to pass a "collection" for the MultipleSelect Filter to work correctly. Also each option should include a value/label pair (or value/labelKey when using Locale). For example:: { filter: type: FilterType.multipleSelect, collection: [{ value: true, label: 'True' }, { value: false, label: 'False'}] }`); } -const optionCollection = this.columnDef.filter.collection || []; +const optionCollection = this.columnDef.filter.collection || this.columnDef.params.collection || [];

You can keep the optionCollection within the editor if you want but the one in the interface should really be collection to align with the already implemented Filters. As mentioned in my previous post, we can revisit that in 2.x

I didn't think about the customStructure but yeah it also makes sense to put it in the params as well. I understand the filter principle but still I would rather make a perfect cut between the 2 and not see anything about filter within an editor, it might confuse some users to do that, however params is generic enough not to confuse them.

do you want me to ignore the filterOptions that I was creating and pass no options? Or should there be a property on the params object?

- const filterOptions = (this.columnDef.filter) ? this.columnDef.filter.filterOptions : {}; - const options: MultipleSelectOption = { ...this.defaultOptions, ...filterOptions }; - his.$filterElm = this.$filterElm.multipleSelect(options);

oh I guess that would be another thing that we can put inside the params and make the Editor and Filter share it, when provided. We can update the Filter in a separate PR as you suggested, but for Editor, it would be

+ const selectOptions = (this.columnDef.filter) ? this.columnDef.params.options : {}; + const options: MultipleSelectOption = { ...this.defaultOptions, ...selectOptions }; + his.$filterElm = this.$filterElm.multipleSelect(options);

not sure about options though, it might be too generic. How about domOptions or elementOptions?

i can see elementOptions because it is for the actual dom element, but do we need to specify it is for the collection element? collectionOptions?

I think it's more for the multiple-select DOM element, so I would go more with elementOptions or domElementOptions but it's prob too long

I can go with elementOptions. The only downside I can see with this approach is if there were ever other elements in the column, then they would potentially have to share this elementOptions, but this might be how you want it designed since most DOM elements wont share many properties.

It will be a bit before I update this PR, but I will have it to you later this evening.

ghiscoding · 2018-03-05T00:14:48Z

aurelia-slickgrid/src/examples/slickgrid/example3.ts

+      type: FieldType.string,
+      editor: Editors.multipleSelect,
+      filter: {
+        collection: Array.from(Array(1001).keys()).map(k => ({ value: `Task ${k}`, label: `Task ${k}` }))


goes with the other comment, I would rather see params.collection instead of seeing filter.collection to be used in both multiSelect.

ghiscoding · 2018-03-05T03:54:36Z

oh gosh I feel embarrassed, I thought it was live review, I didn't know that I had to send the review once it's completed. feel dump oopsy

jmzagorski · 2018-03-05T15:13:07Z

I'll try to get in the revisions at some point today or later tonight. I will not be in the office today and I will have limited access to my computer.

ghiscoding · 2018-03-05T16:01:10Z

Sure whenever is best for you, no rush. However if you can't make it by tomorrow, then let me know and I'll cut a release tonight instead. Thanks

also make error message and editor property names more meaningful

jmzagorski · 2018-03-05T21:43:46Z

Okay I think I changed everything, but let me know if I missed something. I will be unavailable for the next few hours.

jmzagorski · 2018-03-05T21:56:00Z

One more thing I'll need your opinion on that i just realized. The editors assume that the data item is a foreign key reference and not a complex object with a label and value property, see example 3 Where I set the data for those properties.

Is this a bad assumption and should I support a list of values and a list of complex objects with a key and value? Hopefully that makes sense.

ghiscoding

1 very small missing curly brace, the rest looks good. I will give it a try later after work and cut a release tonight then. Thanks

ghiscoding · 2018-03-05T22:05:19Z

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

-          'Locale) is required to populate the Select list, for example:: { filter: ' +
-          'type: FilterType.multipleSelect, collection: [ { value: \'1\', label: \'One\' } ])');
+          'Locale) is required to populate the Select list, for example: ' +
+          '{ collection: [ { value: \'1\', label: \'One\' } ])');


You added an open curly brace in your throw error, though I think it's missing an ending curly brace.

ghiscoding · 2018-03-06T02:14:53Z

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

+      container: 'body',
+      filter: false,
+      maxHeight: 200,
+      width: '100%',


you should not fix this to 100%, it makes the option "autoDropWidth" unusable. The "autoDropWidth" is a new option that I added which will resize the drop menu with the same size as the input select element.

ghiscoding · 2018-03-06T02:17:22Z

Now that I am testing it locally, here are some observations

intellisense doesn't work on params, since for sure since it's type is any. I know I am the one who suggested to use params but I'm not totally in love with that anymore. However to help, we can pass an extra type to params: ExtraOptions | any | any[] where ExtraOptions would be a new interface that includes elementOptions: SelectOption and collection: any[] but if we do that, then we might as well add a new Column property.
1. it's totally my bad, I should have thought about the intellisense much earlier. If we do add a new Column property, I would like to make it a generic to include both Editor/Filter and possibly other piece of code that might use a collection.
2. question. Do you want to keep params for now and come back with another property later? Or do you want to create a new Column property now?
fixing the width: 100% is not a good idea, it makes the option autoDropWidth unusable. The autoDropWidth is a new option that I added to the 3rd party lib, which will resize the drop menu with the same size as the input select element, it's convenient in some cases like on the last column when the option list is narrow (options of True/False are quite narrow so we can use it there).
1. however the following settings make it more usable in the UI, elementOptions: { width: 150, offsetLeft: 20 }, see the print screens below
2. autoDropWidth and offsetLeft are 2 options that I personally added to the multiple-select.js which is why it's in my repo in the lib folder, you can see these options here
it would also be better if you use the same defaults as the multipleSelectFilter, the translation key are of particular interest because if they are omitted then the 3rd party lib (in this case multiple-select.js) will use it's own titles (only English). So please provide them.
clicking on a cell value (even with autoEdit enabled) requires 2 clicks, 1x to open the cell value for edit and another 1x to open the multiple select options. However by looking at the multiple-select.js code, I found a open method (I would have been surprised not to find one), and so we can use it
1. note it doesn't seem to work without the setTimeout and it's a bit slow to open, though I still think we should leave it. What do you think?
2. ah wait I understand why it's slow, there's 1000 options (I would suggest to trim down the demo to 500 items) in the multiple select to create, if there's only 10 then it would be super fast to open. So I suggest to add this open within the editor.

this.$editorElm = this.$editorElm.multipleSelect(options);
+ setTimeout(() => {
+   this.$editorElm.multipleSelect('open');
+ }, 0);

Before (no options passed, use the defaults that include width: 100%)

After (with elementOptions: { width: 150, offsetLeft: 20 })

jmzagorski · 2018-03-06T03:03:24Z

Thank you for the feedback. I will look this over and answer your questions a bit later when I am near my computer. Not sure how late I am able to work tonight so you can cut a release without this if you like.

ghiscoding · 2018-03-06T03:08:41Z

I can wait another day, no hurry.
No one complained about the other bug yet so it's ok to wait another day.

jmzagorski · 2018-03-06T04:05:59Z

question. Do you want to keep params for now and come back with another property later? Or do you want to create a new Column property now?

If you are not opposed, lets keep params since there are a lot of changes in this PR already. Then we can have another PR focused on just updating the API for the Editor and Filter

it would also be better if you use the same defaults as the multipleSelectFilter,

I think this might be a larger issue because we do not have control over creating an editor to inject the i18n library. The only way for dependency injection to work in an Editor is for the user to inject the editor into their view model using Factory.Of or (which is better in my opinion) we wrap their columnDef.editor property in Factory.of in the AureliaSlickGridCustomElementor somewhere in the library.

ghiscoding · 2018-03-06T04:17:23Z

Yes sure, we can always create more PR. I am not opposed to that at all, I always prefer to ask :)

You can also use params to pass the i18n this is actually the preferred way of doing it in my Wiki - localization with Formatters. That is what I do for Formatter and it will work perfectly with Editor as well, you just use the instance without DI. The good old javascript way. Welcome back to the javascript world lol ;)

Also feel free to add some info on things you would like to correct in version 2.x in this issue #19
I just added a note to possibly remove Column filter option and replace it by a common property.

ghiscoding · 2018-03-06T15:58:12Z

Since I have that in mind, I wanted to tell you yesterday that the dateEditor does use the i18n in it. Which is like I told you, I pull the i18n from the params.i18n, it has to be written that way and it is in my Wiki docs.

also create an exact width so it plays nice with the multipl-select library

jmzagorski · 2018-03-06T17:17:38Z

Okay let's see if i fixed all your notes:

Editor will use params object for now until the new params interface is defined in v2.0
I set the width of the multiple-select to the values you provided and it looks good
Added i18n translation in both editors so the behavior is similar to the filter.

Opened an issue to further discuss pros/cons of allowing DI in editors since the library uses i18n for some editors. We therefore would not be dependent on the user to pass this.

Per your suggestion I added the setTimeout and open method and it behaves a lot nicer since the select lists opens immediately. However, after doing some research, this does not seem to be easy with the fallback HTML single select element so I left it as is. Finally, I limited the drop down to Task0 - Task10 because for the demo.

ghiscoding · 2018-03-06T17:54:23Z

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


  constructor(private args: any) {
+    this._i18n = this.args.column.params.i18n;


this will probably fail at Build, args could possibly be undefined here

ghiscoding · 2018-03-06T17:54:42Z

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


  constructor(private args: any) {
+    this._i18n = this.args.column.params.i18n;


same as before.. this will probably fail at Build, args could possibly be undefined here

ghiscoding · 2018-03-06T17:56:43Z

Awesome this looks really nice, I tried it over lunch and yes you did addressed all the things I said.
The last thing would be to see if errors comes up in Build Lib, I'm expecting that you might have some errors with possibly undefined

BTW, the offset is only required on the last column, in the middle it's not really needed.

jmzagorski · 2018-03-06T18:21:23Z

I ran the build and I did not receive any errors. Did you see errors on your end?

For the offset, should I have to somehow dynamically set that value if it is the last column or always setting that value the way I did it won't hurt anything?

ghiscoding · 2018-03-06T18:43:53Z

I didn't see any errors, I assumed the Build would throw an error. If it's ok then fine

feat(selectEditors): add select grid editors

ad2a7e3

select editors are common with data to ensure data integrity

docs(demo): add singleSelect and multiSelect to example3

81be263

jmzagorski requested review from ghiscoding and removed request for ghiscoding March 5, 2018 03:26

ghiscoding reviewed Mar 5, 2018

View reviewed changes

refactor(editors): use params not filter columnDef prop

8219631

also make error message and editor property names more meaningful

ghiscoding reviewed Mar 5, 2018

View reviewed changes

ghiscoding reviewed Mar 6, 2018

View reviewed changes

refactor(editors): add i18n and open select on cell click

8c32223

also create an exact width so it plays nice with the multipl-select library

ghiscoding reviewed Mar 6, 2018

View reviewed changes

ghiscoding merged commit 5bd7215 into master Mar 7, 2018

ghiscoding deleted the feat/select_editors branch March 10, 2018 23:13


		constructor(private args: any) {
		this._i18n = this.args.column.params.i18n;

feat(selectEditors): add select grid editors #22

feat(selectEditors): add select grid editors #22

Conversation

jmzagorski commented Mar 3, 2018 • edited Loading

ghiscoding commented Mar 3, 2018 • edited Loading

jmzagorski commented Mar 3, 2018

ghiscoding commented Mar 3, 2018

jmzagorski commented Mar 3, 2018

ghiscoding commented Mar 5, 2018

jmzagorski commented Mar 5, 2018

ghiscoding commented Mar 5, 2018

jmzagorski commented Mar 5, 2018

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 Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghiscoding Mar 5, 2018 • edited Loading

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

ghiscoding commented Mar 5, 2018

jmzagorski commented Mar 5, 2018

ghiscoding commented Mar 5, 2018

jmzagorski commented Mar 5, 2018

jmzagorski commented Mar 5, 2018

ghiscoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghiscoding commented Mar 6, 2018 • edited Loading

jmzagorski commented Mar 6, 2018

ghiscoding commented Mar 6, 2018

jmzagorski commented Mar 6, 2018

ghiscoding commented Mar 6, 2018 • edited Loading

ghiscoding commented Mar 6, 2018

jmzagorski commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghiscoding commented Mar 6, 2018 • edited Loading

jmzagorski commented Mar 6, 2018 • edited Loading

ghiscoding commented Mar 6, 2018

jmzagorski commented Mar 3, 2018 •

edited

Loading

ghiscoding commented Mar 3, 2018 •

edited

Loading

jmzagorski Mar 5, 2018 •

edited

Loading

ghiscoding Mar 5, 2018 •

edited

Loading

ghiscoding commented Mar 6, 2018 •

edited

Loading

ghiscoding commented Mar 6, 2018 •

edited

Loading

ghiscoding commented Mar 6, 2018 •

edited

Loading

jmzagorski commented Mar 6, 2018 •

edited

Loading