version 2.x refactoring to support Multiple Grid about aurelia-slickgrid HOT 108 CLOSED

Dohhhh I feel like Omer Simpson now... I didn't realized that we do use the editor today lol
I guess we use it that way because that is what SlickGrid likes to see. Wow I'm sorry, I was missing an important piece since the start of the subject, my bad. I'm glad we talked because I would have missed it completely, now I understand the editor: c.editor.slickEditor it totally make sense

in that case, from your previous post. I think I would go with option number 1

this.grid = new Slick.Grid(`#${this.gridId}`,
       this.dataview,
       // if we made the Editor class property name "slickEditor"
       this._columnDefinitions = this._columnDefinitions.map(c => ({ ...c, editor: c.editor.slickEditor, editorOptions: { ...c.editor } })),
       this.gridOptions);

from aurelia-slickgrid.

I'll leave it as a 2 formatters for now (CollectionFormatter and CollectionEditorFormatter), I'll let you play with that before the release. I would say this is minor and can wait.

from aurelia-slickgrid.

okay, I will see if it even works or makes things simpler

from aurelia-slickgrid.

Actually, it only works after I did the change. Can you update the numberFilterCondition.ts with this code

export const numberFilterCondition: FilterCondition = (options: FilterConditionOption) => {
  const cellValue = parseFloat(options.cellValue);
  let searchTerm = (Array.isArray(options.searchTerms) && options.searchTerms[0]) || 0;
  if (typeof searchTerm === 'string') {
    searchTerm = parseFloat(searchTerm);
  }

+  if (!searchTerm && (!options.operator || options.operator === '')) {
+    return true;
+  }
  return testFilterCondition(options.operator || '==', cellValue, searchTerm);
};

Try it out and see if that is the behavior you want, you can tweak it if necessary.

We might also have to revisit the other conditions

from aurelia-slickgrid.

Right, formatters are more challenging. Two methods I can think of off the top of my head right now:

1. You would have to define an interface for the user to use so your library knew what function to call on the class after it was instantiated
2. Or the user would have to create a class with their dependencies as the first parameters to their class constructor and then factory.of feeds the rest of the arguments to the constructor, similar to the editor. Then when slickgrid runs it will be calling this Aurelia's invoke every time (not sure what type of overhead that brings)
   For example:

@inject(i18n)
export class MyFormatter {
  constructor(i8n i8n, row, cell, value, columnDef, dataContext) {
  }
}

I am sure there are better ways and I can think of some as I am going through my code.

from aurelia-slickgrid.

Migration Changes Guide from version 1.x to 2.x
The list is starting to be too big, so I created a Wiki for it

from aurelia-slickgrid.

@jmzagorski
How do you want to address number 7. and 13.

from aurelia-slickgrid.

For number 7 i think you wanted to keep the editor and filter configuration separated in the columnDef. Probably a good idea since both accept a collectionFilterBy property and I have a grid that dynamically filters the params.collection by changing the columnDef.params.collectionFilterBy on cell click, while leaving the filter.collection alone. Not sure if that can be accomplished if both the editor and filter use the same configuration?

That would mean leave the editor on the params or move them to an editor specific property (in reference to number 13)

from aurelia-slickgrid.

@jmzagorski
I would like to put some Types for Editors, not just for Filters. The problematic is even though we can use filter, we can't use editor since it is used internally by SlickGrid itself. We could call it editorOptions though, and if so, we could potentially rename filter to filterOptions to make them equal (however, I understand that would create more refactoring in the migration, but I'm ok with that).

The end goal is to get some Types (and intellisense when the coding editor supports it, like VScode). I wanted to wait until Filters and Editors were stable enough, I think they are now, and for version 2.x before revisiting them. I think we have everything we need to make a decision, which I kinda need rather soon because I already started the Major version work in my Angular repo.

I don't think that leaving all of the Editors stuff in params is a good idea, especially if we want more TS Typing.

from aurelia-slickgrid.

Here are a few other options I could think of to brainstorm about:

1. Instead of filterOptions, editiorOptions etc, make a property called options and then put filter and editor as properties (columnDef.options.filter, columndDef.options.editor)

2. Make your public API columnDef.editor a Editor | EditorOptions so it is similar to the filter. Then in AureliaSlickGridCustomElement move the editor option properties to another property on the columnDef and put the actual Editor interface on the original columnDef.editor for slickgrid to use.

from aurelia-slickgrid.

I was thinking about your number 1. too, the downside is that it makes the property calls longer. Unless you want to merge Editor/Filter properties and only separate the ones that you want? But that might be confusing for collection stuff

This kinda mean that Editor and Filter will have the exact same collection behavior. Is that always wanted?

options: {
       collection: [ 
         { value: '', label: '' }, 
         { value: true, label: 'true' }, 
         { value: false, label: 'false' },
         { value: undefined, label: 'undefined' }
       ],
       collectionFilterBy: {
          property: 'effortDriven',
          operator: OperatorType.notEqual,
          value: undefined
       },
       collectionSortBy: {
          property: 'effortDriven',    // will sort by translated value since "enableTranslateLabel" is true
          sortDesc: false,             // defaults to "false" when not provided
          fieldType: FieldType.boolean // defaults to FieldType.string when not provided
       }, 
       type: FilterType.multipleSelect
   }

Your number 2. I'm not sure to follow you on how to fake editor. Are you saying to replace editor before passing the column definitions to the new Slick.Grid? I feel that it's kind of a patch

from aurelia-slickgrid.

I think combing it might be troublesome, but that is just a guess. For example, in my app I dynamically filter the singleSelectEditor when a cell is clicked, but the header filter still shows all the values from the same collection. I do this because each row can only have a subset of values from the entire collection based on another value in the same row. For example:

    onCellClick: ({ columnDef, dataContext }) => {
      columnDef.params.collectionFilterBy = {
        property: 'jobTitleId',
        value: dataContext.ownerJobTitleIds,
        operator: OperatorType.contains
      };
    },

So if this was changed every time a cell was clicked, will this affect the header filter? I know the header filter is rendered early on so it might not affect it unless the header filter updates while the user is using the grid.

As for number 2, yes it is sort of "hacky" i guess. The only thing it does is makes your public API cleaner and not tied to slickgrid anymore. Here is a brief example of how it could work without changing the underlying column definitions:

    this.grid = new Slick.Grid(`#${this.gridId}`,
       this.dataview,
       // if we made the Editor class property name "slickEditor"
       this._columnDefinitions.map(c => ({ ...c, editor: c.editor.slickEditor })),
       this.gridOptions);

from aurelia-slickgrid.

hmm maybe with the spread operator it doesn't look too bad. What is c.editor.slickEditor? I mean where does that come from? Aren't we just supposed to pull whatever the user added in editor and empty it out afterward we're done keeping it in a temp area that will be used by our EditorService?

from aurelia-slickgrid.

c.editor.slickEditor would be what columnDef.editor is today, i just gave it a random name so probabaly should use another name if we decided to use this method (maybe call it type like filter.type does?). For example:

Today

{
    editor: Editors.date,
    field: 'due',
    filter: {
      type: FilterType.compoundDate
    },
    filterable: true,
    formatter: Formatters.dateIso,
    id: 'due',
    name: 'Due',
    sortable: true,
    type: FieldType.dateIso
  }

Brainstorm

{
    editor: {
      slickEditor: Editors.date,
     // ... other editor options here
    },
    field: 'due',
    filter: {
      type: FilterType.compoundDate
    },
    filterable: true,
    formatter: Formatters.dateIso,
    id: 'due',
    name: 'Due',
    sortable: true,
    type: FieldType.dateIso
  }

from aurelia-slickgrid.

but where is the part to swap the property in a temp variable? Isn't your .map supposed to be reassigned to column definitions after you're done? SlickGrid uses editor for it's Editor Factory, so we need to empty it out before creating the grid itself. If we use editor that can be renamed to a temp variable (let say editorOptions), isn't it suppose to look like the following?

this.grid = new Slick.Grid(`#${this.gridId}`,
       this.dataview,
       // if we made the Editor class property name "slickEditor"
       this._columnDefinitions = this._columnDefinitions.map(c => ({ ...c, editor: { }, editorOptions: c.editor })),
       this.gridOptions);

BTW, I'd like to have the brainstorm done by the end of this week and implement it in the weekend and then in my Angular repo on Monday. I need multiple grids in my project, like now lol

from aurelia-slickgrid.

oh yeah i forgot that part, and I forgot about the editorFactory property on gridOptions. So we can blank out the editor and put our factory on gridOptions.editorFactory.getEditor or reassign the editor and editorOptions on the fly

If you were to use editorOptions the reassignment would look like this:

this.grid = new Slick.Grid(`#${this.gridId}`,
       this.dataview,
       // if we made the Editor class property name "slickEditor"
       this._columnDefinitions = this._columnDefinitions.map(c => ({ ...c, editor: c.editor.slickEditor, editorOptions: { ...c.editor } })),
       this.gridOptions);

If you were to use editor factory it would look like this:

-for (const c of this._columnDefinitions) {
-      if (c.editor) {
-        c.editor = Factory.of(c.editor).get(this.container);
-      }
-}
+ this.gridOptions.editorFactory {
+  getEditor: column => Factory.of(this.columnDefinitons.find(c => c.id === +column.id).editor.slickEditor).get(this.container)
+}

this.grid = new Slick.Grid(`#${this.gridId}`,
       this.dataview,
       // if we made the Editor class property name "slickEditor"
+       this._columnDefinitions = this._columnDefinitions.map(c => ({ ...c, editor: null, editorOptions: { ...c.editor } })),
       this.gridOptions);

from aurelia-slickgrid.

When I said Editor Factory, I really meant SlickGrid internal factory which is the column editor property, you can see it used here by the getEditor() function. But yeah we also have our own Editor Factory, which you did an excellent job at implementing :)

The editor: c.editor.slickEditor still confuses me, I don't know what it does since I believe the editor should be left empty or null as you pointed out in 2nd sample.
BTW, editorFactory is also a column property used internally by SlickGrid here, so we can't use that option either :P

Which one do you think would be the best option to go with?

from aurelia-slickgrid.

If the editor is left null then we must implement the editorFactory. If we do not want to use a factory then we must set the editor property for slickgrid to use as we do today. editor: c.editor.slickEditor is a made up property for this example if we were to have your API use c.editor for the actual editor (e.g DateEditor, TextEditor etc) and the editor options.

{
    // today we do 
-   editor: Editors.date
    // but if you wanted to use editor as an object instead for the actual editor class & options
+   editor: {
+      slickEditor: Editors.date,
+      collection: this.editorCollection
     // ... other editor options here
    },
-    params: {
-      collection: this.editorCollection
-    }
  }

Not sure if that helps any. I don't have a preference as it stands now.

I was thinking about your number 1. too, the downside is that it makes the property calls longer.

I wouldn't worry about that if you use the destruction operator. The destruction would make it columnDef.options.editor.collection to options.editor.collection

from aurelia-slickgrid.

@jmzagorski
I want to make sure that you see number 4.b and 4.c from the list on top. I would like to make a better separation of the dispatch events of SlickGrid versus the Aurelia-Slickgrid ones. Basically, I want SlickGrid to start with prefix sg, while the Aurelia-Slickgrid events to start with asg. This would help the user and us if there's any troubleshooting to do, to understand where the event is processed. Make sense?

from aurelia-slickgrid.

Makes sense yes. What does your html view on your left-side menu look like? You might be able to catch the event in your view if there is a parent element to your grid without using the EA.

from aurelia-slickgrid.

Ah the example that was written is with my current App, however it's almost identical to our demo with left-side menu, the only difference is that the left-side menu is collapsible. I can re-code our demo to look similar and add the collapsible. The idea is that the left-side menu and the right-side are containers, they don't know what goes in, neither do they have a relationship of Parent/Child, they are independent.

See the print screen below, the "x" is clickable which will collapse and only show the icons and not the text. When that "x" is clicked, it doesn't fire an DOM resize events, I think it's due to the fact that I only use a CSS property change and not an actual element resize... all that to say, I had to go with a Global Event Emitter (which in the Aurelia is an Event Aggregator)

from aurelia-slickgrid.

@jmzagorski
I am almost done with the refactoring of editor, I decided to go with the following

this._columnDefinitions = this._columnDefinitions.map((c: Column | any) => ({ ...c, editor: this.findEditor((c.editor && c.editor.type), c), internalColumnEditor: { ...c.editor } })),

Some notes

1. To make the editor to behave exactly like filter, I created a new interface EditorType which can be used inside the editor.type (exactly like filter).
   • it is a bit different than our last discussion since it doesn't take an actual SlickGrid editor, it is rather an enum of EditorType. This way it is similar to Filter and also brings Custom Editor in the play
2. instead of using editorOptions to internally swap the options, I decided to use a more familiar name internalColumnEditor which tells the user to not use that property
3. the function findEditor will be a Factory call in our Aurelia-Slickgrid, I couldn't find how to create a factory in Angular and so I just did a quick and dirty collection find instead of a factory.
4. the editor is of type ColumnEditor (just like filter was of type ColumnFilter) and this brings all the usual properties that were previously used the generic params
5. with the new EditorType, I was able to easily add a custom and created a Custom Editor and it... sweet
   • however, I can't new it since it has to a valid SlickGrid class (not an instance) because if I do, I won't get access to the args required in the constructor (provided internally by SlickGrid)
6. very important there is only 1 issue that still stand, the CollectionFormatter is using the Collection from the generic params and now it's no longer available. I can take it from the editor but wait I can't... why? because I don't know if the Collection is inside the editor or the filter object!?!
   • it's also not just the collection but also customStructure and possibly more in the future,
   • so for now, I left it inside params for the Example 3 (Editor) to still work.
   • I need your input for this one

The commit for all of that in the Angular-Slickgrid repo can be seen here and I will release a beta version in that repo in the coming days

from aurelia-slickgrid.

I am pretty sure CollectionFormatter should be from the editor because that function formats values selected from the editor.

however, I can't new it since it has to a valid SlickGrid class (not an instance) because if I do, I won't get access to the args required in the constructor (provided internally by SlickGrid)

Not sure what you mean by this. Are you newing up the editor class yourself?

from aurelia-slickgrid.

@jmzagorski
How about renaming the formatter to CollectionEditorFormatter which would get it's collection from the editor complex object while maybe leaving untouched CollectionFormatter and still associated to params

The new is what we currently do with a Custom Filter as shown here

{
  id: 'description', name: 'Description', field: 'description', filterable: true, sortable: true, minWidth: 80,
  type: FieldType.string,
  filter: {
    type: FilterType.custom,
    customFilter: new CustomInputFilter() // create a new instance to make each Filter independent from each other
  }
},

while a Custom Editor looks like this, the only difference is that we can't new it up

{
  id: 'title2', name: 'Title Custom', field: 'title',
  sortable: true,  minWidth: 70
  type: FieldType.string,
  editor: {
    type: EditorType.custom,
    customEditor: CustomInputEditor
  },
}

and here's a regular Editor with the new complex editor object

{
  id: 'prerequisites', name: 'Prerequisites', field: 'prerequisites',
  sortable: true,  minWidth: 100
  type: FieldType.string,
  editor: {
    type: EditorType.multipleSelect,
    collection: Array.from(Array(12).keys()).map(k => ({ value: `Task ${k}`, label: `Task ${k}` })),
    collectionSortBy: {
      property: 'label',
      sortDesc: true
    },
    collectionFilterBy: {
      property: 'label',
      value: 'Task 2'
    }
  }
}

from aurelia-slickgrid.

I update the migration guide for the editor complex object, you can see the guide

from aurelia-slickgrid.

okay I see what you mean with the CollectionEditorFormatter. If the user does not have an editor, they still might need to use the CollectionFormatter.

What about the CollectionFormatter checking if there is an editor then using the editor.collection property and if not fall back to the params.collection?

from aurelia-slickgrid.

@jmzagorski
I was thinking about the fallback but then if the user also has the collection inside the filter, then what? which one has priority and such? Also the if condition is gonna be huge, take a look at the if code now. I would prefer to simply create multiple formatters, it's easy to create and doesn't add much more code, it's also more focused and less confusing

from aurelia-slickgrid.

Right now the CollectionFormatter ignores the filter.collection and always uses the params.collection which is only used by the editor, so would we have to worry about filter.collection?

from aurelia-slickgrid.

@jmzagorski
The thing is that the collection is now inside the editor object, no longer inside params. I think you knew that already, but just making sure you had that info.

A formatter is meant to format the data, so there's nothing anywhere that says it is meant to be used in combo with the Editor. Having a new CollectionEditorFormatter is much more explicit, and we can leae the CollectionFormatter with params, in case you want to use it without any editor or filter.

from aurelia-slickgrid.

okay. I see what you are doing. If you did not want to create a formatter every time there is a collection under a different column property, you could create a factory function

export const collectionFormatter: Formatter = (columnProperty) => (row: number, cell: number, value: any, columnDef: Column, dataContext: any) => {
  if (!value || !columnDef || !columnDef[columnProperty] || !columnDef[columnProperty].collection
    || !columnDef[columnProperty].collection.length) {
    return '';
  }

  // etc. replace every params with [columnProperty] which can be filter or editor or whatever else
};

View model would be

const columnDef = [{
  formatter: Formatters.collection('editor')
}]

Just an idea, you dont have to use it.

from aurelia-slickgrid.

oh that would be nice indeed, but TS seems to not like the way you wrote.

I also tried like this

export const collectionFormatter = (columnProperty: string): Formatter => (row: number, cell: number, value: any, columnDef: Column, dataContext: any) => {

now it doesn't make any errors but columnProperty is undefined

from aurelia-slickgrid.

oh yeah, because the columnDef.formatter will return a Formatter or a Func<string, Formatter>

UPDATE: Oh i read that wrong. The actual formatter function would return a Func<string, Formatter> instead of a Formatter

from aurelia-slickgrid.

@jmzagorski
Sorry I'm going to have to bother you again, once this is done, I think the Major Version will be ready for a Beta spin.

So I was working late on implementing the editor with the same code as what I have in my Angular repo. Pushed a commit. But I'm having some issues with this piece of code at this line

this._columnDefinitions = this.columnDefinitions.map((c: Column | any) => ({ ...c, editor: this.getEditor((c.editor && c.editor.type), c), internalColumnEditor: { ...c.editor } }));

fails because c.editor is not resolved yet I believe because of you Factory.of loop that you've put in the bind here,

Also, I just wanted to get it going but eventually I want to replace the getEditor() function here by a Factory function. I didn't find how to do that in Angular and didn't want to spend time, but that should be easy to do in Aurelia. Once that is done the AVAILABLE_EDITORS can go away.

Again this is the last stretch, I've put all the fixes and changes that I've done from my Angular repo to here. we are getting close... :)

from aurelia-slickgrid.

Actually thought about it, we just have to move the reassignment of the column definition into the bind() function and also we can use the Factory.of right from there. However there is an exception, the EditorType.custom won't work since it's not part of the factory and has to be loaded with column.editor['customEditor']

If you have the chance to look into that during the day, that would be great.

from aurelia-slickgrid.

Was there a reason why you made the columnDef.editor interface a editor?: EditorType | ColumnEditor; instead of a editor?: Editor | ColumnEditor and then the ColumnEditor.type property would be the editor class passed by the user as we do it in version 1? Then you dont need a factory since they are passing us the class

from aurelia-slickgrid.

right that is probably why I need to make it a type any somewhere in my code. it is a mistake and yes it should be editor?: Editor | ColumnEditor

As for the type, it wasn't working with the type being something else than enum. Also, the advantage of enum is that it brings the custom editor as a new type. I wanted to make the editor have the same structure as filter and by using an enum, it is the same structure. You can try with type as Editor but when I tried it was failing on me. If we keep the factory, we can change the EditorType enum to have the key equal the value (checkbox = 'checkbox', ...) so that it can be used by the factory.

I'm still in Beta on my Angular repo (until next week) as well, so I don't mind changing implementation if need be

from aurelia-slickgrid.

I was just curious.

Is there any benefit to using an enum over the actual class being passed to type? Looking at the custom filter implementation:

filter: {
       type: FilterType.custom,
       customFilter: new CustomInputFilter() // create a new instance to make each Filter independent from each other
    }

It makes the filter interface more verbose and also makes more work in your library to instantiate the one they want to use based on an enum when the users could just pass either your filter or their custom one:

filter: {
+     type: Filters.select // or new CustomInputFilter()
-       type: FilterType.custom,
-       customFilter: new CustomInputFilter() // create a new instance to make each Filter independent from each other
    }

Just a thought and this would apply to the edit too

from aurelia-slickgrid.

I tried changing the type as you said to editor?: Editor | ColumnEditor and it works, here's the commit in Angular repo. I can't work on the Aurelia one until after work.

The only benefit in using the enum is that it doesn't pass the entire class when you import EditorType versus the Filters (basically if we do Editors.checkbox, like we did in version 1). I don't mind changing both the Filter and Editor if it helps to make the library lighter and less complex, will the DI still work though? For the Custom Editor, I couldn't do the new since it was conflicting with SlickGrid as I wrote here

from aurelia-slickgrid.

the DI should still work. We would change the inner workings of the filterFactory and just use this.container.get(columnDef.filter.type) since the filter.type will be a class export instead of a name. That would remove the need for the user to remember to register their custom filter as a plugin for this library and there would be no need for a filterType property on the class.

from aurelia-slickgrid.

sounds good, if you have a chance to do, it would help out a lot. Else I might have few questions to get it going

from aurelia-slickgrid.

@jmzagorski
so I tried to move the column definition reassignment into the bind() function and that worked. Here's the code without the comments.

bind() {
  this.gridOptions = { ...GlobalGridOptions, ...this.gridOptions };
  this._columnDefinitions = this.columnDefinitions.map((c: Column | any) => ({ ...c, editor: this.getEditor((c.editor && c.editor.type), c), internalColumnEditor: { ...c.editor } }));

  this.columnDefSubscriber = this.bindingEngine.collectionObserver(this.columnDefinitions)
    .subscribe(changes => this.updateColumnDefinitionsList(this._columnDefinitions));

  for (const c of this._columnDefinitions) {
    if (c.editor) {
      c.editor = Factory.of(c.editor).get(this.container);
    }
  }
}

Also I made a mistake in the sg-on-cell-changed.delegate it should be change not changed, that is to show the item changes in the blue alert.

I did not commit any changes (I did other fixes in other files though), I only wrote it here for reference. I also thought you might be in the middle of playing with the code on your side as well. Any luck with the refactoring you mentioned earlier? I'll let you push your changes tomorrow (Friday), it would be really nice to get a Beta release by early next week.

from aurelia-slickgrid.

Also, you can simplify the bind if you want:

bind() {
  this.gridOptions = { ...GlobalGridOptions, ...this.gridOptions };
  this._columnDefinitions = this.columnDefinitions.map((c: Column | any) => ({ 
    ...c,
-    editor: this.getEditor((c.editor && c.editor.type), c),
+   editor: c.editor && Factory.of(this.getEditor(c.editor.type), c)).get(this.container),
    internalColumnEditor: { ...c.editor } 
  }));

  this.columnDefSubscriber = this.bindingEngine.collectionObserver(this.columnDefinitions)
    .subscribe(changes => this.updateColumnDefinitionsList(this._columnDefinitions));

-  for (const c of this._columnDefinitions) {
-    if (c.editor) {
-      c.editor = Factory.of(c.editor).get(this.container);
-    }
-  }
}

from aurelia-slickgrid.

Also do you think internalColumnEditor is a little verbose since the property is in the columnDef object (as opposed to just internalEditor)? (lol I know I analyze everything)

@ghiscoding
ALSO
Is there a reason you imported all the editors in https://github.com/ghiscoding/aurelia-slickgrid/blob/feature/new-major-2-release/aurelia-slickgrid/src/aurelia-slickgrid/models/columnEditor.interface.ts if they are not being used in the file?

from aurelia-slickgrid.

the name is of the type that goes in (currently it's any but it should be of type ColumnEditor as shown in my Angular commit, so I just added internal in the front :P
It might help also to not get confused with this being a SlickGrid editor (which is not). I don't mind changing the name though, it's not to be used outside anyway

As for all the Editors imported, it's most probably a mistake from the first draft to get it working. I don't think we need them anymore, you can delete them

from aurelia-slickgrid.

ok. As for the name i think internal is good, I was talking about column in internalColumnEditor. Do you think it would be better naming it internalEditor instead of internalColumnEditor since the property is on the column object already.

from aurelia-slickgrid.

I don't mind changing the name if it bothers you, if you are in the middle of code change, can you make sure that these changes displayed in this Angular repo commit are also part of our Aurelia lib. Thanks

from aurelia-slickgrid.

it does not bother me since it is internal, i just thought I would bring it up for discussion to see if it sounded redundant or not

from aurelia-slickgrid.

On the ColumnFilter interface there is an operator property and in your filter.service you are selecting the operator based on the FilterType property here for non-custom filters.

Since I am trying to make the Editor and Filter interface match (where both type properties are the class and not enum), this switch logic is problematic.

We could remove the switch logic and encapsulate the operator value as a read-only property in the Filter interface since the operator should always be the same according to your comment here (e.g the SingleSelectFilter will have a property called operator and that will have a value of 'EQ'. )

Not sure if that is what you want, but it is one way to reduce code in this service and add more encapsulation if the filter's operator will always be the same publicly. I see you do use 'IN' here, but that seems more internal to clear out values since you do not want to use 'IN' when actually filtering a SingleSelect. If the operator is on the interface, then custom filters have to put it there so you can access the operator on any filter using filter.operator

However I am not that familiar with operator elsewhere.

from aurelia-slickgrid.

The operator cannot be read only, it is often set to be 1 operator for a lot of the regular Filters but now all of them. It is dynamic when using Compound filters and can also be changed when using a regular string filter (you can write *Doe and that will return every string that finishes with Doe (John Doe, Jane Doe). We could also add more Compound filters in the future, for example I'd like to have CompoundMultipleSelect (which would have an IN and NIN as operator that you could choose (there's always a default Operator though, on a multiple select, it has to be IN by default).

Here are the possible Operators showing for a Compound Date Filter

from aurelia-slickgrid.

ahh that's right. Do you have any suggestions for that switch logic in the FilterService if Filter.type is changed from FilterType to Filter? You could possibly run a switch statement on the class signature, for example:

        switch (filterType) {
-          case FilterType.select:
+         case Filters.select:
-          case FilterType.multipleSelect:
+         case Filters.multipleSelect:
            operator = 'IN';
            break;
-          case FilterType.singleSelect:
+         case Filters.singleSelect:
            operator = 'EQ';
            break;
          default:
            operator = operator;
            break;
        }

However that is assuming the Filter.type is a class signature and not an instance. This looks like the last hurdle in changing the type property on both the Editor and Filter

from aurelia-slickgrid.

isn't it better to simply add a default operator type inside each Filters?

For the code you wrote, can we actually do a switch case on a class? what does typeof Filters.select return?

from aurelia-slickgrid.

The typeof would return Function for all of them so I dont think that would work.

The defaultOperator would work since that logic is only for an operator that is not already set, and you would not have to have the addition logic to check if it is a custom filter because you would be able to handle custom default operator types:

+     operator = operator ? operator : columnDef.filter.defaultOperator;
-     const filterType = (columnDef.filter && columnDef.filter.type) ? columnDef.filter.type : FilterType.input;
-     if (!operator && filterType !== FilterType.custom) {
-       switch (filterType) {
-         case FilterType.select:
-         case FilterType.multipleSelect:
-           operator = 'IN';
-           break;
-         case FilterType.singleSelect:
-           operator = 'EQ';
-           break;
-         default:
-           operator = operator;
-           break;
-       }
-     }

Not sure if you like that? Then we could get rid of the FilterType all together. I see you do add a type to _columnFilters array in this class, but I don't see it used anywhere?

Also I think editor?: Editor | ColumnEditor should be editor?: any | ColumnEditor right? Because Editors.longText, Editors.text are the class exports and not an instance. I say that because i get compile errors in example 11 when I changed type to type: Editors.longText. Maybe I am doing it wrong.

from aurelia-slickgrid.

The operator seems good the way you wrote it, however you can simplify it a bit. I mean code wise, the result will be the same, but you can write it this way too

-     operator = operator ? operator : columnDef.filter.defaultOperator;
+     operator = operator || columnDef.filter.defaultOperator;

Well actually, you can only write it that way if operator is never 0 and never false, else you have to use the ternary operator.

The editor?: Editor | ColumnEditor is working in my Angular repo but if you are in the middle of getting rid of EditorType and going with Editors, then I guess you'll have to use any. I'm not sure why but TS doesn't seem to like that we put the type Editor, technically it should work since LongTextEditor implements Editor or is extending, I forgot.

from aurelia-slickgrid.

I think TS is expecting an instance on the type property if it is editor?: Editor | ColumnEditor right? and Editors.longText is not an instance of Editor, but is a class export because slickgrid is creating the instance for us.

from aurelia-slickgrid.

Yeah it's probably something like it, too bad that we have to put any to go around the issue though.

Are you gonna be able to push your code by the end of the day?
Hey I also meant to ask before but kinda forgot, how did your presentation go with the grid?
Are they satisfied with it, is it missing anything feature wise?

from aurelia-slickgrid.

Yes, I will try to get it in tonight, if not tomorrow. I will probably break it up into 2 commits (editor then filter)

The presentation went well, they like the grid. The only feature I know about is the ability to show/hide their columns and save that configuration (I can't remember if you implemented that. If you did I have not had time to put it in my app). Basically I have a Dashboard page that has your grid and they can choose from a bunch of different columns so they can hide/show which they want). Since the app is so new they might have more requests once it is in production. ill add some screen shots and explain a little more how I am using the grid in that documentation issue.

from aurelia-slickgrid.

funny, I'm actually looking at this right now (visibility) lol
BTW it's not implemented, I opened this issue #67 to cover it

from aurelia-slickgrid.

okay i pushed the editor change. I may get to the Filter tonight or tomorrow (i am dealing with a webpack+DllPlugin+karma+aurelia issue for my unit tests ). I'll let you review that commit in case you don't like the direction that change went.

from aurelia-slickgrid.

I reviewed quickly your last commit and I can see that it was exactly the first approach that I went before trying to make the structure look like Filter.

I guess it's ok for the new path, however I think that type is no longer a good word and I don't want previous user to think it is an EditorType or FilterType since it's no longer a type per say. So can we have another property name, perhaps something like class? or another word you think is better, but not type, especially for Filters if you are to do the same structure as this new Editors

I made a small commit in Example 3, you might want to pull before going with further changes.

from aurelia-slickgrid.

hmmm, naming...it is the hardest part of coding. I would say class, but technically someone can use a regular old function, and it is not an "instance" of a class either. It is really the constructor/named export

slickgrid calls it IEditor here, so we could just say IEditor or interface?

from aurelia-slickgrid.

The new standard is to not use an "I" in the front of an interface anymore, so I wouldn't use that. As for interface, I'm not sure this is good either since it's more of a class that is instantiated while an interface is just a structure per say.

from aurelia-slickgrid.

The new standard is to not use an "I" in the front of an interface anymore

That is what I get for not keeping up with typescript! Thanks for updating me.

i thought about class, but the user is passing the library a non-instantiated class (which would not make it a class, but a constructor function) because Slickgrid is instantiating it for us here. Maybe that is getting too technical

from aurelia-slickgrid.

How about classReference? I don't know what else to use, however I'm still ok with class personally. A Class doesn't have to be instantiated to exist as a Class, if it was instantiated I would call it instance.

If that is still not good, then I don't know, maybe a word from this list: accessor, builder, package, container, engine, composer, template, member, metaclass, mixin, prototype, model (this one is not so bad)

from aurelia-slickgrid.

The only other one I could think of was ctor (for constructor). class is a reserved es6 keyword so I don't know if that is a good idea. classReference is good, but we might want to document that they dont have to use an actual es6 class and can use a plain old JavaScript function constructor like in the slickgrid editors file. model is very appealing too since it is a shorter name and it goes with your naming convention. By that i mean if a user didnt know what editor.model was they might figure it out since you have an editor interface file in the models folder? Lol idk. I think I actually like that one the best.

I won't be able to work on this or get on github until later today so let me know what you decide and I can start the changes later.

from aurelia-slickgrid.

Let's go with model then, I didn't want to use type because I want to make it obvious that both Filters and Editors are used differently compare to 1.x.

On my side, I think that I'm almost done with Grid State & Presets for all states (filter, sorter, pagination, column visibility, size & position). I have to test with the backend, which I'll do tomorrow. However, this branch is based on the new major 2 branch, so I need your code done before I can merge it anywhere.

Tomorrow is ok too, if you can finish it during the day. I'd like to make a Beta Monday or Tuesday night. That is if we are finished and are happy with the changes. Also the Migration Guide will have to be updated with your latest change.

from aurelia-slickgrid.

On the compoundDateFilter and compoundInputFilter you already have an operator property. Do you want me to use this property for all filters to set the default filter value instead of defaultOperator?

from aurelia-slickgrid.

Yeah it's normal for a Compound Filter to have an operator since it's dynamic and can change, so that one is the correct name. For the question, should we use operator everywhere? Hmm I don't have a preference, but I guess to make them all look the same, it's probably better to use operator as well indeed. I'll leave it up to you.

from aurelia-slickgrid.

I was testing the filters with my changes and noticed a possible bug on exampe5. If I filter a*=e and then select either male or female, the grid returns no data and I do not see any exceptions.

However on this branch i get

Unhandled rejection TypeError: Cannot read property 'toLowerCase' of undefined

here because the columnId is "and gender", which does not exist in the column array (only gender does)

from aurelia-slickgrid.

That's ok, Example 5 is displaying data but I had to do a lot of work to fake an OData sample. It is not representing the reality of an OData Server. However, we can fix this easily by moving the column[columnId] right after the for loop and use a variable to keep it as a string const col = column[columnId] + ''; and replace all the return with that variable. We now know it's a string, so we can call the toLowerCase() without exception.

By the way, why would you filter a*=e? is that with a Compound Filter of a* and a search value of =e? That's like having 2 operators, which one will win?

from aurelia-slickgrid.

sorry, i meant a* filter with a value of e. If it is just in the example, that is fine then.

from aurelia-slickgrid.

yeah it's just an example, it's kinda hard to repro a complete OData Server lol.
but that filter you've put, shouldn't throw an exception though. Unless you have null data in that column?

from aurelia-slickgrid.

if you put a* filter with a value of e and then select a gender, then it throws

from aurelia-slickgrid.

Ok I made a quick patch, you can replace the for loop with this

for (const columnId in columnFilters) {
  if (columnFilters.hasOwnProperty(columnId)) {
    filteredData = filteredData.filter(column => {
      const filterType = columnFilters[columnId].type;
      const searchTerm = columnFilters[columnId].term;
      let colId = columnId;
      if (columnId && columnId.indexOf(' ') !== -1) {
        const splitIds = columnId.split(' ');
        colId = splitIds[splitIds.length - 1];
      }
      const filterTerm = column[colId];
      if (filterTerm) {
        switch (filterType) {
          case 'equal': return filterTerm.toLowerCase() === searchTerm;
          case 'ends': return filterTerm.toLowerCase().endsWith(searchTerm);
          case 'starts': return filterTerm.toLowerCase().startsWith(searchTerm);
          case 'substring': return filterTerm.toLowerCase().includes(searchTerm);
        }
      }
    });
  }
}

This example never worked in the past with multiple filters, now it does :)

from aurelia-slickgrid.

So I am done with my changes, but I am making sure I did not break anything because I had to make a lot of changes. However, I am noticing one difference between this branch and master. In example 4 when you clear all filters and then filter on the complete column with [for example] > 50 and then you remove the > and 50 the grid does not show all records again until you click "Clear all filter". Do you see this as well?

Here a quick gif. This is from checking out a clean branch without my changes:

from aurelia-slickgrid.

@jmzagorski
It's because of the default operator that is set in the filter condition here, if no operator is passed, it will use == which I thought would be the correct behavior if you start typing a number, you would expect it to be equal.

However, this might be more confusing, so you can add this before the return

if (!options.operator || options.operator === '') {
    return true;
}

but I'm not sure if that is the correct behavior either. The testFilterCondition doesn't really have a contain (it has IN but that is for a string contain). If you put 50 without an operator, will all the user expect to see nothing filtered? Probably not.

from aurelia-slickgrid.

At the end of the GIF when I clear 50 and the operator, shouldn't that clear the filters so I can see all the data? (I should have hit clear filters again in the GIF to show that all records were not shows when I cleared the operator and 50.)

from aurelia-slickgrid.

I just saw your GIF, it's a bit different than I understood. Yes it should put back all the data and it does in my current branch of the Grid State Columns, however there's a lot of fixes related to the filters, sorters in that branch. So I would say, push your code in and I will try to merge the 2 branches together (ideally make a PR to merge the new grid state branch into the major branch, then 1 final PR to include all of it)

So keep it in mind and let's revisit that issue after the 2 branches are merged together

from aurelia-slickgrid.

okay. I have a lot of changes in mine too, do you want me to create a branch so you can look it over (i can create a PR into this branch)? I did some things I was not sure about. Or do you want me to commit on this branch?

from aurelia-slickgrid.

@jmzagorski
did you work on the same branch new-major-2-release? If so, I can take a look at the commits, hopefully less than 10 :P

I also need a way to put all your changes into my Angular-Slickgrid repo, so hopefully it won't take me too long ;)

Or if it's a different branch, you can create a PR to merge on the major branch

from aurelia-slickgrid.

I committed it all in one commit on this branch so we can revert it if you do not like it.

The one thing I was not sure about was my change in the graphql.service which removed the mapOperatorByFilterType here. I tested on example 6 and everthing seemed to work because the columnDef.filter.operator always had the correct operator.

The one thing I thought about was forcing transient registration in the FilterFactory because if a user does not register their filter as Transient, it will default as singleton and probably not act right. However, I left this out in case there is an edge case where someone wants singleton.

Side note: I am sure you are aware of this, but the date filters on example 12 do not filter based on a similar format filter value (see gif below), probably because of the underlying value being a date object vs string filter value. Just thought I would mention it while I was looking it over:

from aurelia-slickgrid.

Example 12 should also use the Compound Date Filter, I can update that later. Technically what happens is that when you add the "-", it becomes an invalid date and that is why there's nothing returned

from aurelia-slickgrid.

@jmzagorski
That was an amazingly long day, it took me couple of hours to release all of this and it seems that NPM has some issues showing my Beta version. I had similar issues on my work computer, so I guess I will wait few hours to see. NPM says that it got publish for version 2.0.0 with a Beta tag but I still don't see it

Anyhow, here's the last info on the project

1. merged both branches grid-state-columns and major-release-2
2. pushed to master
3. updated all client-cli and github/doc
   • client-cli throws an error with flatpickr when switching locale, e.g. on Example 12, click on "switch language" and it throws this error:

Error: Module name "flatpickr/dist/l10n/fr.js" has not been loaded yet for context: _

the error comes from this line. Was the error there before? It's possible, but somehow this require doesn't work with loading the locale with RequireJS
4. updated GitHub demo page and also added Example 15 with Grid State & Presets with Local Storage
5. note I did not use the regular conventional-changelog because I wanted to create a Beta tag without doing a Major, however I created a release with few info, you might want to review it.

There's only 1 major thing missing and it's to update the Migration Guide with the new changes you've put in for Editors/Filters. The rest should be covered. Of course we will have to update all the Wikis but that can be progressive.

EDIT
Not sure why NPM doesn't show the Beta version but I tried modifying the package.json of the GitHub demo to download ^2.0.0 and it works. So it's probably just NPM that is delayed.

Let me know when you upgrade your work version, I would like to know if it all works after following the Migration Guide.

I am planning to release the official version in a week or 2, if it all goes well. And as I mentioned in the release, it's in Beta, so if we need to change something, it is still possible to do so. Thanks for all your help 💯

from aurelia-slickgrid.

Great job! As for the flatpickr error I never ran into it, but I only use English (however my app will eventually have to be translated into many different languages.)

Do you want me to work on the migration guide for the editor/filter changes I committed?

from aurelia-slickgrid.

@jmzagorski
Yes the migration is basically only missing the Editors/Filters changes you made yesterday. The rest should be covered and is well documented, I think

from aurelia-slickgrid.

I will work on that. My plan this week is to work on the migration documentation, fix all the unit tests in my app to make sure everything works (my app was also in the process of going from V1 (Kendoui grid) to V2 (your grid)). Then I will close out our long documentation issue and integrate AureliaSlickgrid V2-Beta to see if anything works differently than expected.

from aurelia-slickgrid.

Sweet that is a nice plan :)
Hope our lib is better than KendoUI hehe

oh and don't delete the major-release-2 branch, I need it to see the commits you made to port it over my Angular-Slickgrid repo. Thanks

from aurelia-slickgrid.

@jmzagorski
I found a new bug that annoys the hell out of me, I spent already half a day on it and I just can't figure out what causes it. The problem is that even if I clear (or change sort) and then I clear of change a filter, sometimes the original sorting comes back!?! I can always reproduce it if I go on Example 4, do a Clear Sorting and right after a Clear Filters, the sorting comes back, wth! From the investigation so far, I found that it comes from this line dataView.refresh() inside the filter callback. Also it puts back the sort icon, only after reaching the callback of that column (basically from the sample below, after triggering a clear filter, it will start showing the sort icon only after reaching the "Duration" column). I have a feeling that it's because of a reference pointer and possibly since our refactoring to support multiple grid.

I don't have an older version anymore, so I can't really test this. Or perhaps, I could use the Github demo and use previous Aurelia-Slickgrid version but I would have to remodify the example 4 again.

from aurelia-slickgrid.

Good thing I am behind on my pulls. My master is 9 commits behind, the last commit I have is refactor(demo): update to latest version 1.13.0 and this problem does occur so it is not in our new stuff

from aurelia-slickgrid.

Found the issue: the clear filters calls dataView.onRowCountChanged which the sort.service subscribes to here. The args.current from that event call is 478, which is greater than 0 so the local presets are loaded on the grid.

UPDATE:
I will keep my master version where it is in case anything else comes up.

from aurelia-slickgrid.

oh ok good thing I asked then, did you just find the issue or do you know how to fix it?

Actually, I think I know what to do, I should simply clear the presets.sorters after clearing the sorting... and that works!!! Thanks man, I was banging my head on this one. I don't think that we need the presets once they are loaded on first page load.

from aurelia-slickgrid.

Just found it. Not sure how much work I will be able to do tonight.

from aurelia-slickgrid.

ah it's ok, there's no rush. I wanted to push a Beta and it is done. I just encounter coupe of small issues, like that one, while testing other things in my work project. We can now use the master branch again with 2.x going forward, so making small PR is back to the table :)

from aurelia-slickgrid.

@jmzagorski
I'm refactoring the Editors to support validators. I didn't know until today that we can watch grid.onValidationError to deal with the result of the validate() function in the Editors and display that validation to the end user.

My question to you is the following, can we pass a custom validator to the editor property in the Grid Definitions? I doesn't work in my Angular repo because I didn't refactor the code that you've done recently with model in editor and filter, but would that work in Aurelia-Slickgrid?

Basically a custom validator would look like this

const myCustomTitleValidator = (value) => {
  if (value == null || value === undefined || !value.length) {
    return { valid: false, msg: 'This is a required field' };
  } else {
    return { valid: true, msg: null };
  }
};

and pass it to the definitions

this.columnDefinitions = [
  { id: 'title', field: 'title', editor: { model: Editors.longText }, validator: myCustomTitleValidator }
];

I didn't try it since I'm not in Aurelia, but I wish it works. Can you confirm?

EDIT
Actually when I started writing it, I thought validator was inside editor but then I realized it was actually inside the Column properties, so yeah it will work.

from aurelia-slickgrid.

@jmzagorski
not sure if it's really a good topic or not but I was looking at the implementation of Editors and saw the factory.of on this line. This makes me thing, we could possibly do that also with the formatters to have DI working and get rid of the i18n in the grid options. I think it's only Formatters that aren't DI yet. For example the translateFormatter needs the i18n from grid options, as shown on this line but if we had DI that would fix that formatter.

Hmm actually, our formatters are not classes, not sure if that would implicate a lot more changes on the end user side of it?

from aurelia-slickgrid.

@jmzagorski
Found a bug with the loading of Filter Presets, if you go on Example 4 you will see the problem. On that example, it's suppose to load search terms "2, 22, 44" but it only loads data with "2". The problem comes from this line, the searchTerms has the 3 values but the operator is empty string and so will do an equal since it's empty, so only first value filter (2) will show up. It's also because this line got removed and so there's no longer any operator defined.

Any suggestions on how to fix this?

I think it should be this

this._columnFilters[columnDef.id] = {
        columnId: columnDef.id,
        columnDef,
        searchTerms,
-        operator: (columnDef && columnDef.filter && columnDef.filter.operator) ? columnDef.filter.operator : null
+        operator: (columnDef && columnDef.filter && columnDef.filter.model.operator) ? columnDef.filter.model.operator : null
      };

actually that seems to cause other problems hmmm

from aurelia-slickgrid.

what if you move the creation of the filter here up higher in the code to here so the operator logic can be

operator = columnDef.filter.operator || filter.operator;

Then you can either pass it to updateColumnFilters or add it to the columnDef.filter.operator.

from aurelia-slickgrid.

@jmzagorski
Yeah that is a lot better and it simplifies the code too, thanks.

from aurelia-slickgrid.

@jmzagorski
oops talked too fast, Example 4 was ok but Example 12 broke since columnDef.filter is undefined. I changed your line to this

operator = filter.operator || (columnDef && columnDef.filter && columnDef.filter.operator);

is that ok?

or the inverse, I don't know which one should be checked first?

operator = (columnDef && columnDef.filter && columnDef.filter.operator) || filter.operator;

from aurelia-slickgrid.

that should be fine unless columnDef.filter.operator should take precedence over filter.operator, then you would want to flip the equation.

from aurelia-slickgrid.

Yeah I see that in the in ColumnFilter, the operator can be used and maybe defined in the column definition, so I will flip it. Also a filter can be undefined, so I have to take that in consideration too.

So the final result is this

addFilterTemplateToHeaderRow(args: { column: Column; grid: any; node: any }) {
    const columnDef = args.column;
    const columnId = columnDef.id || '';

    if (columnDef && columnId !== 'selector' && columnDef.filterable) {
      let searchTerms: SearchTerm[] | undefined;
      let operator: OperatorString | OperatorType;
+      const filter: Filter | undefined = this.filterFactory.createFilter(args.column.filter);
+      operator = (columnDef && columnDef.filter && columnDef.filter.operator) || (filter && filter.operator) || undefined;

      if (this._columnFilters[columnDef.id]) {
        searchTerms = this._columnFilters[columnDef.id].searchTerms || undefined;
        operator = this._columnFilters[columnDef.id].operator || undefined;
      } else if (columnDef.filter) {
        // when hiding/showing (with Column Picker or Grid Menu), it will try to re-create yet again the filters (since SlickGrid does a re-render)
        // because of that we need to first get searchTerm(s) from the columnFilters (that is what the user last entered)
        searchTerms = columnDef.filter.searchTerms || undefined;
-        this.updateColumnFilters(searchTerms, columnDef);
-        operator = columnDef.filter.operator || undefined;
+        this.updateColumnFilters(searchTerms, columnDef, operator);
      }

I finally took the time to port it over to my Angular repo, which is how I find this issue.

from aurelia-slickgrid.