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

feature(grid): ability to dynamically add or change Column Headers #65

Merged
merged 2 commits into from
May 21, 2018

Conversation

ghiscoding
Copy link
Owner

- We can dynamically add or change column definitions, see Example12 for a demo
@ghiscoding
Copy link
Owner Author

ghiscoding commented May 19, 2018

@jmzagorski
Looked at the open issues and saw that the issue #42 to change default binding on dataset might also expand to current PR. If so, can you make the changes or tell me what to change. Thanks

I ran a build and was amazed to see only 2 small errors, wow usually I have to fix like 30 undefined variables or whatever lol

Anyway, I want this PR in before I push a new release. Do you have anything else in the pipeline before a release? Let me know please, I'd like to release in the coming days. Thanks

@jmzagorski
Copy link
Collaborator

with #42 all you have to do is change this:

-@bindable({ defaultBindingMode: bindingMode.twoWay }) dataset: any[];
+@bindable dataset: any[];

The reason: twoWay implies that AureliaSlickGridCustomElement will or can update the dataset and the changes will be propagated back to the view model (hence the name two-way). I believe, by default, custom elements have a binding of oneWay (from the view model to the custom element).

I do not think we will ever be need to mutate the dataset...right?

Copy link
Collaborator

@jmzagorski jmzagorski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great feature! In my application I have a custom element called redux-grid that uses your library and I had to implement a columnDefinitionsChanged method because I was changing columns. I did not have to use BindingEngine because I implement redux which means when something in my column changes, the entire array is a new instance and the columnDefinitionsChanged fires. It seems that I know can remove my implementation.

@ghiscoding
Copy link
Owner Author

would you mind refactoring directly in the branch? I'm going out now and that would help if you can apply the change directly. Thanks

@jmzagorski
Copy link
Collaborator

will now close #42

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 20, 2018

I do not think we will ever be need to mutate the dataset...right?

but technically, a user could come and change the JSON dataset, I actually also do that with the OData backend service I think, which in that case is an empty array at first but then receives the data once backend replies (Example 5 is coded that way, if that example still works then I guess it's ok). We also need to support data insert (Example 11, in that case we push to the dataset/dataView. Unless I misunderstood, don't we still need a two way binding for that to work?

If both Example 5 and Example 11 still works, then I would assume that we are good. Let me go and try it on same branch... ok both examples are working fine.

I guess we are good to go then. Any last comment?
Also I wrote in the other PR, do you have anything else in the pipeline or are we good for a release in the coming days? We have a long weekend in Canada, so I might do a release on Monday

@jmzagorski
Copy link
Collaborator

It is fine if the user/viewmodel changes the dataset since those changes will always flow into the AureliaSlickgridCustomElement, but if AureliaSlickGridCustomElement class changes the dataset (ie changes the instance to another variable) then that change won't update the viewmodel dataset. As you found out in the examples everything should still work as expected

Nothing more in the pipeline from me right now.

@ghiscoding
Copy link
Owner Author

ok good, so you said you can close #42 since it was incorporated in this PR ?

@jmzagorski
Copy link
Collaborator

Yes #42 can be closed after this is merged

@ghiscoding ghiscoding merged commit 0eca219 into master May 21, 2018
@ghiscoding ghiscoding deleted the feature/dynamically-add-column-header branch May 30, 2018 21:55
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.

None yet

2 participants