Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sort): add a Clear Sorting function and grid menu command #39

Merged
merged 1 commit into from
Apr 4, 2018

Conversation

ghiscoding
Copy link
Owner

No description provided.

@ghiscoding
Copy link
Owner Author

@jmzagorski
Another PR if you have time to review, or I can merge it and release on Monday/Tuesday. Not a problem if you can't review, I know you're quite busy :)

@ghiscoding
Copy link
Owner Author

ghiscoding commented Apr 4, 2018

@jmzagorski
FYI and in case you haven't noticed, I'm starting to simplify each Services init() function (or whatever function name is used for initializing the service). I don't want to do these changes all in 1 shot, so I do them when I touch them and test them. Basically, what I started to do is eliminate gridOptions and columnDefinitions and keep only grid and dataView objects.

Why the refactoring?
Because it's much simpler to deal with only 2 arguments instead of 4-5 args and because we can easily get the gridOptions and columnDefinitions from the grid object anyway.

For example, this PR has the following changes

Aurelia-Slickgrid.ts
// attach external sorting (backend) when available or default onSort (dataView)
if (gridOptions.enableSorting) {
-  (gridOptions.backendServiceApi || gridOptions.onBackendEventApi) ? this.sortService.attachBackendOnSort(grid, gridOptions) : this.sortService.attachLocalOnSort(grid, gridOptions, this.dataview, this.columnDefinitions);
+ (gridOptions.backendServiceApi || gridOptions.onBackendEventApi) ? this.sortService.attachBackendOnSort(grid, dataView) : this.sortService.attachLocalOnSort(grid, dataView);
}

Then in the SortService, it now changes to this

SortService.ts
-  attachLocalOnSort(grid: any, gridOptions: GridOption, dataView: any, columnDefinitions: Column[]) {
+  attachLocalOnSort(grid: any, dataView: any) {
+    this._isBackendGrid = false;
     this._grid = grid;
-    this._gridOptions = gridOptions;
+    this._dataView = dataView;
+    let columnDefinitions: Column[] = [];
+
+    if (grid) {
+      this._gridOptions = grid.getOptions();
+      columnDefinitions = grid.getColumns();
+    }

I think it worth to know about and I will merge this PR on Wednesday evening when I'm preparing the release. This should help in simplifying, though I'm not sure if it helps in any kind for the 2 grids in 1 page issue.

@jmzagorski
Copy link
Collaborator

that makes sense and might make it easier to refactor when reviewing the possibility of multiple grids. Thanks for the update

@ghiscoding ghiscoding merged commit f709dc7 into master Apr 4, 2018
@ghiscoding ghiscoding deleted the feature/clear-sort branch April 9, 2018 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants