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

fix(eventService): use grid.getOptions for gridDefinition #51

Merged
merged 2 commits into from
May 14, 2018

Conversation

jmzagorski
Copy link
Collaborator

This fixes a potential problem where the grid options as a parameter in the method are different than grid.getOptions. Users are allowed to change options after the grid is created by calling, for example, grid.getOptions().showHeaderValue = false. To make sure the options are up to date we need to call grid.getOptions when passing it as event args.

Looking over AureliaSlickgridCustomElement, it should be fine to use this.gridOptions because most of the configuration is happening when the custom element is being created. The only potential issue I could see is if the datasetChanged fires after the grid is created. Then any call to this.gridOptions could potentially be out of data if the developer allows the user to change options later on. You can see this bug in example3 when you switch autoEdit off and click the "edit" button. The gridDefinition has autoEdit=true when it should be false

I was not sure if you wanted to use args.grid or just the grid param because I see you use both so I just picked one.

closes #49

This fixes a potential problem where the grid options as a parameter in the method are different than grid.getOptions. Users are allowed to change options after the grid is created by calling, for example, `grid.getOptions().showHeaderValue = false`;. To make sure the options are up to date we need to call `grid.getOptions` when passing it to the events. Looking over `AureliaSlickgridCustomElement`, it should be fine to use `this.gridOptions` because most of the configuration is happening when the custom element is being created. The only potential issue I could see is if the `datasetChanged` fires after the grid is created. Then any call to `this.gridOptions` could potentially be out of data if the developer allows the user to change options later on. You can see this bug in example3 when you switch `autoEdit` off click the "edit" button. The gridDefinition has `autoEdit=true`

closes #49
@ghiscoding
Copy link
Owner

ghiscoding commented May 1, 2018

Yeah that will be good to use, since I want to eventually convert all services to only pass the grid and dataView objects as arguments, we can get the grid options and column definitions from the grid object. Here's an example with the SortService

However, the gridOptions is no longer referenced in the argument list and if we delete it, this could be a breaking change for some. I added a reference to remove this argument in next version 2.x.

You are right about the grid vs args.grid, I guess they will always be equal and we can use any of them. The fact that we use the fat arrow => allows us to use grid object, else we would use args.grid.
I'm wondering if we could drop the arguments all together, since most of them are available in the args, though the dataView might not be available in the args? Could you check and if it's in the args, I would remove them all now and not wait for 2.x

@ghiscoding
Copy link
Owner

ghiscoding commented May 12, 2018

@jmzagorski
I reviewed this PR and branch and decided that we might as well refactor right away (instead of waiting for 2.x) which is what I wrote in previous post

You are right about the grid vs args.grid, I guess they will always be equal and we can use any of them. The fact that we use the fat arrow => allows us to use grid object, else we would use args.grid.
I'm wondering if we could drop the arguments all together, since most of them are available in the args, though the dataView might not be available in the args? Could you check and if it's in the args, I would remove them all now and not wait for 2.x

The gridOptions, which was previously the 2nd argument, was only really filled in directly by aurelia-slickgrid component and since your original refactoring, this argument is no longer used anyway. So I deleted this argument entirely. I also removed all references to args.grid and replaced by grid.

Please review and go ahead with the merge if you think it's all good. That is 1 less service that has gridOptions as arguments.

@jmzagorski jmzagorski merged commit 0be30fd into master May 14, 2018
@jmzagorski jmzagorski deleted the fix/stale_gridEventSvc_gridoptions branch May 14, 2018 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gridOptions are out of sync with grid.getOptions()
2 participants