feat(event): #24 add onGridStateChanged and onPaginationChanged events #28

ghiscoding · 2018-03-10T22:54:57Z

add onGridStateChanged
add onPaginationChanged event that Angular-Slickgrid can then trigger Grid State onGridStateChanged event
add better onFilterChanged & onSortChanged events

- add onGridStateChanged - add onPaginationChanged event that Angular-Slickgrid can then trigger Grid State onGridStateChanged event - add better onFilterChanged & onSortChanged events

ghiscoding · 2018-03-10T22:56:40Z

@jmzagorski
Would you mind doing a quick review of the Grid State events PR I just made.
I also modified the Grid State wiki and added a section at the end (with a note saying it will be available in next version). The new section is Grid State Events.

You can also try it by pulling the branch feature/grid-state-events

I'm planning on releasing a new version on Monday night.
Could you please also fix the bug #27 which is causing a few problems in the examples with presets.

I also need this to be merged before starting the work on Compound Filter since it changes code in Filter Service which this PR also does.

jmzagorski · 2018-03-11T04:17:01Z

👍 i plan on doing some work tomorrow so I can look at the bug and this PR

jmzagorski · 2018-03-11T14:55:54Z

aurelia-slickgrid/src/aurelia-slickgrid/slick-pagination.ts

@@ -171,6 +171,17 @@ export class SlickPaginationCustomElement {
    } else {
      throw new Error('Pagination with a backend service requires "BackendServiceApi" to be defined in your grid options');
    }
+
+    // dispatch the changes to the parent component
+    this.elm.dispatchEvent(new CustomEvent(`on-pagination-changed`, {


it might be good if we always use the eventPrefix on dispatched events so we know the events will be unique to the library. That prefix is in the AureliaSlickgridCustomElement, but it might be good if we put it in a separate constants file (of if another applicable file exists) so other custom elements can use it too

I didn't want to use the sg prefix because it represents the SlickGrid events not Aurelia-Slickgrid events. I didn't feel the need to have a prefix for internal one. Do you really think it's necessary?

I prefixed the events with sg to uniquely identify any event coming from your library that is exposed to the user (i see this in many aurelia libraries). Since these events are bubbling from your library they will always be exposed to the user. In my application I can bind to that on-pagination-changed event and any other event, and if we prefixed some events and not others that are exposed I thought that may come across as inconsistent. For example:

<template> <aurelia-slickgrid on-pagination-changed="handlePagination()" sg-on-cell-click="handleClick()"> </aurelia-slickgrid> </template>

hmm I would still rather make a distinction between my own events versus the SlickGrid events which always have an event and args and are not working the same as internal ones.

Okay I understand. You may still want a different prefix because another library can have the same event name on-pagination-changed that would be in conflict with yours since they all bubble up the DOM

ok perhaps as (Aurelia-Slickgrid), hmm no that's too confusing lol...
or asg that's probably better

jmzagorski · 2018-03-11T14:56:56Z

aurelia-slickgrid/src/aurelia-slickgrid/aurelia-slickgrid.ts

@@ -286,6 +294,14 @@ export class AureliaSlickgridCustomElement {
      }
    }

+    // expose GridState Service changes event through dispatch
+    this.gridStateSubscriber = this.ea.subscribe('gridStateService:changed', (gridStateChange: GridStateChange) => {
+      this.elm.dispatchEvent(new CustomEvent(`on-grid-state-service-changed`, {


same comment about the eventPrefix

jmzagorski · 2018-03-11T14:57:40Z

aurelia-slickgrid/src/examples/slickgrid/example4.html

@@ -3,6 +3,6 @@ <h2>${title}</h2>
  <div class="subtitle" innerhtml.bind="subTitle"></div>

  <aurelia-slickgrid grid-id="grid1" dataview.bind="dataview" grid.bind="gridObj" column-definitions.bind="columnDefinitions"
-    grid-options.bind="gridOptions" dataset.bind="dataset">
+    grid-options.bind="gridOptions" dataset.bind="dataset" on-grid-state-service-changed.delegate="gridStateChanged($event.detail)">


same comment about the eventPrefix

jmzagorski · 2018-03-11T14:58:00Z

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

@@ -152,6 +152,10 @@ export class Example4 {
    }
  }

+  gridStateChanged(gridState) {
+    console.log(gridState);


Are you going to put some logic here?

it's just for demo purposes

ooh didn't realize it was on example4.ts

jmzagorski · 2018-03-11T15:45:25Z

aurelia-slickgrid/src/aurelia-slickgrid/services/filter.service.ts

+    // we also need to add our filter service (with .bind) to the callback function (which is outside of this service)
+    // the callback doesn't have access to this service, so we need to bind it
+    const self = this;
+    this._slickSubscriber.subscribe(this.attachBackendOnFilterSubscribe.bind(this, self));


since you are binding this to the scope of attachBackendOnFilterSubscirbe you do not need const self = this

The this that is in there seems to be in fact the SlickGrid event and args, which is why I added the self. I haven't tried (this, this) though but that would look funny

You only need this._slickSubscriber.subscribe(this.attachBackendOnFilterSubscribe.bind(this));. Then your attachBackendOnFilterSubscribe will have the context of your service already, so using this within the method will work.

ok will give that a try a bit later

you were right about this, I didn't know that simply doing the .bind(this) was enough. Good to know and it cleans up the code a lot. Thanks :)

jmzagorski · 2018-03-11T16:06:40Z

aurelia-slickgrid/src/aurelia-slickgrid/services/sort.service.ts

+    // subscribe to the SlickGrid event and call the backend execution
+    // we also need to add our filter service (with .bind) to the callback function (which is outside of this service)
+    // the callback doesn't have access to this service, so we need to bind it
+    const self = this;


same comment about being able to remove self

jmzagorski · 2018-03-11T16:07:28Z

aurelia-slickgrid/src/aurelia-slickgrid/services/sort.service.ts

@@ -38,6 +47,7 @@ export class SortService {
      backendApi.preProcess();
    }
    const query = backendApi.service.onSortChanged(event, args);
+    self.emitSortChanged('remote');


when self is removed this can be this.emitSortChanged('remote')

actually no, the function is a callback (of the Filter DOM element) and has no knowledge of this service anymore, which is why I needed to add another self argument

Right, it is a callback, but you are already binding this to the callback. So if you do this._slickSubscriber.subscribe(this.attachBackendOnSortSubscribe.bind(this)); you can do this.emitSortChanged('remote') I tested it out and it worked.

jmzagorski · 2018-03-11T16:07:42Z

aurelia-slickgrid/src/aurelia-slickgrid/services/filter.service.ts

@@ -77,6 +79,9 @@ export class FilterService {
    // call the service to get a query back
    const query = await backendApi.service.onFilterChanged(event, args);

+    // emit an onFilterChanged event
+    self.emitFilterChanged('remote');


when self is removed this can be this.emitFilterChanged('remote')

jmzagorski · 2018-03-11T16:09:37Z

aurelia-slickgrid/src/aurelia-slickgrid/services/sort.service.ts

   * Other services, like Pagination, can then subscribe to it.
   * @param sender
   */
-  emitSortChangedBy(sender: string) {
-    this._subscriber.subscribe(() => this.onSortChanged.publish('sortService:changed', `onSortChanged by ${sender}`));
+  emitSortChanged(sender: 'local' | 'remote') {


same comment as in the filter service where the logic may be able to be simplified if only two strings are allowed

jmzagorski · 2018-03-11T16:11:31Z

aurelia-slickgrid/src/aurelia-slickgrid/slick-pagination.ts

@@ -85,11 +85,11 @@ export class SlickPaginationCustomElement {
  }

  dispose() {
-    if (this._filterSubcription) {
-      this._filterSubcription.dispose();
+    if (this._filterSubscriber) {


do you need the null/undefined if logic here since you are setting these in the constructor and this will always pass as true?

I actually didn't test the Build Library, but in some cases I think it might be undefined

see if it will build or that is an odd typescript error

ghiscoding · 2018-03-11T22:03:13Z

@jmzagorski
Made a few changes according to your review. Thanks for that, it does help to get both side reviewed, especially in new features. I think I will continue asking you for review for future ones, if you don't mind thanks again.

You can have a final review if you wish, I will merge it on Monday night and release a new version at that time.

Just curious, did you have anything in the pipeline? If so, let me know and I could wait a bit for a new release.

jmzagorski · 2018-03-11T22:20:11Z

Nothing for Monday night most likely. I was in user meetings all last week so I did not get the chance to code a lot. I will work on #25 and hopefully #17 this week for next release

EDIT: I moved those over into the project "To Do" column and will update them when I start working on them this week.

ghiscoding · 2018-03-11T22:52:10Z

@jmzagorski
Thanks for the update.
I really like all the contributions that you have made to this project, I am also happy that you are now a collaborator. Thanks for all. 🎉

I forgot to ask, have you tried doing a branch directly on the project? Are we good without the Dev branch? If so I will delete the TODO notes in the Project. Thx

jmzagorski · 2018-03-11T22:59:25Z

You're welcome, and I appreciate all of your effort to make this such a great library for Aurelia.

You can delete dev. I have been branching off of your master with my feature/fix branches.

feat(event): add onGridStateChanged and onPaginationChanged events

dd71e8d

- add onGridStateChanged - add onPaginationChanged event that Angular-Slickgrid can then trigger Grid State onGridStateChanged event - add better onFilterChanged & onSortChanged events

ghiscoding requested a review from jmzagorski March 10, 2018 22:54

ghiscoding mentioned this pull request Mar 10, 2018

[feature] Grid State Events #24

Closed

jmzagorski reviewed Mar 11, 2018

View reviewed changes

ghiscoding added 4 commits March 11, 2018 17:25

merged(code): move findOrDefault to a function

1a399ec

refactor(event): add aurelia slickgrid event prefix

2a68caa

refactor(code): remove unnecessary self when bind this is enough

5ee0d8e

fix(build): fix Build lib errors of object can be undefined

9cbbc7d

ghiscoding merged commit b657e75 into master Mar 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(event): #24 add onGridStateChanged and onPaginationChanged events #28

feat(event): #24 add onGridStateChanged and onPaginationChanged events #28

ghiscoding commented Mar 10, 2018

ghiscoding commented Mar 10, 2018 •

edited

Loading

jmzagorski commented Mar 11, 2018

jmzagorski Mar 11, 2018 •

edited

Loading

ghiscoding Mar 11, 2018

jmzagorski Mar 11, 2018

ghiscoding Mar 11, 2018

jmzagorski Mar 11, 2018

ghiscoding Mar 11, 2018

jmzagorski Mar 11, 2018 •

edited

Loading

jmzagorski Mar 11, 2018

jmzagorski Mar 11, 2018

ghiscoding Mar 11, 2018

jmzagorski Mar 11, 2018

jmzagorski Mar 11, 2018

ghiscoding Mar 11, 2018

jmzagorski Mar 11, 2018

ghiscoding Mar 11, 2018

ghiscoding Mar 11, 2018

jmzagorski Mar 11, 2018

jmzagorski Mar 11, 2018

ghiscoding Mar 11, 2018

jmzagorski Mar 11, 2018

jmzagorski Mar 11, 2018

jmzagorski Mar 11, 2018

jmzagorski Mar 11, 2018

ghiscoding Mar 11, 2018

jmzagorski Mar 11, 2018 •

edited

Loading

ghiscoding commented Mar 11, 2018

jmzagorski commented Mar 11, 2018 •

edited

Loading

ghiscoding commented Mar 11, 2018 •

edited

Loading

jmzagorski commented Mar 11, 2018

feat(event): #24 add onGridStateChanged and onPaginationChanged events #28

feat(event): #24 add onGridStateChanged and onPaginationChanged events #28

Conversation

ghiscoding commented Mar 10, 2018

ghiscoding commented Mar 10, 2018 • edited Loading

jmzagorski commented Mar 11, 2018

jmzagorski Mar 11, 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

Choose a reason for hiding this comment

jmzagorski Mar 11, 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

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

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

Choose a reason for hiding this comment

ghiscoding commented Mar 11, 2018

jmzagorski commented Mar 11, 2018 • edited Loading

ghiscoding commented Mar 11, 2018 • edited Loading

jmzagorski commented Mar 11, 2018

ghiscoding commented Mar 10, 2018 •

edited

Loading

jmzagorski Mar 11, 2018 •

edited

Loading

jmzagorski Mar 11, 2018 •

edited

Loading

jmzagorski Mar 11, 2018 •

edited

Loading

jmzagorski commented Mar 11, 2018 •

edited

Loading

ghiscoding commented Mar 11, 2018 •

edited

Loading