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

introduce directive property for column filter definition #2918

Closed
wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 5, 2015

This should solve #2796 by adding a 'directive:' property to the filter definition, e.g.:

columnDefs: [{
  name: 'eventData',
  filter: {
    directive: 'my-own-datepicker'
  }}]

If directive is defined than this directive is used to render the filter instead of the default text input field.

So we propose:
a) to add the above directive configuration property
b) to do a 'deep watch' on the 'term' property and allowing arbitrary JavaScript objects on the the 'term' property. This of course only makes sense with external filtering.
c) to allow custom filtering directives to add data to the filter definition. In our case we would like to add a 'type' property that is than also send to the server for server side filtering. Thus the server code nows what filter directive was used and how to interpret the term property.
d) to share the scope with the custom filter directive to make c) possible and it didn't work without that for us.

This pull request is only meant as a proof-of-concept and we'd be glad to also provide documentation and tests if you accept the idea.

Thank you.

@PaulL1
Copy link
Contributor

PaulL1 commented Mar 5, 2015

I think I'm OK with the concept of a custom directive, although we'd need to carefully document what behaviours that directive must provide.

I'm not sure on the deep watch - I presume that's because you want to trigger filtering any time anything in the term changes? Is that instead something we could roll into the directive - so make the directive explicitly call a method when it's ready for the filtering to run? Filtering is reasonably expensive with large data sets, I've never been super happy with running it on every keypress. And we generally try to avoid watches where-ever we can, I think deep watches are things we avoid more than shallow watches. The other guys might have opinions on this too (and they may differ from mine).

We'd probably want to see unit tests with a feature like this, and a tutorial.

But thanks very much for the contribution, it's good to have more contributors committing big chunks of functionality.

@PaulL1 PaulL1 mentioned this pull request Mar 5, 2015
@swalters
Copy link
Contributor

swalters commented Mar 5, 2015

After discussing with contributors, we would rather see colDef.filterTemplate option. This should meet your needs as you can code a directive that is used in the filterTemplate and each column can have a different filterTemplate.

Can you look at approaching from that direction? The first task would be to take the existing code and moving to a default filterTemplate.

ghost pushed a commit to comsolit/ng-grid that referenced this pull request Mar 6, 2015
This is part of angular-ui#2918 to make it possible to use arbitrary complex
custom column filters.

A use case is a date range picker with a term object like
{from: Date, to: Date} or a multiselect.

There should be no measurable performance impact for normal text
filters. Instead of comparing term directly with === angular now
calls angular.equals which starts with

  function equals(o1, o2) {
    if (o1 === o2) return true;
@ghost ghost mentioned this pull request Mar 6, 2015
@pszawlis
Copy link

I tried to apply solution from the fork to the problem described in #2886.

Unfortunately it does not solve the issue with the display, my filter list is still hidden behind ui-grid cells. Is there any special formatting that I should apply for my custom directive?

I tried to reproduce the issue with the plunker substituting standard reference to ui-grid-unstable.js with the ui-grid.min.js built from the fork, but I keep getting this error

"Error: [$injector:unpr] Unknown provider: aProvider <- a <- uiGridHeaderCellFilterDirective"

@PaulL1
Copy link
Contributor

PaulL1 commented Mar 13, 2015

a. I'd try the unminified version from the fork, then you might get a more useful error message
b. There are a set of overflow: hidden attributes set in the grid, you basically have to override all these to get it working. There was another issue recently with this exact same problem, I'll look for it, but I recollect three places - ui-grid-viewport, ui-grid-header and ui-grid-cell or something like that.

@PaulL1
Copy link
Contributor

PaulL1 commented Mar 13, 2015

#2932

@pszawlis
Copy link

Thanks for help, changing the css in the original plunker solved the layout issue, please see amended version of the original plunker. Nevertheless, grid is still not properly redrawn after filtering.

http://plnkr.co/edit/lcoTtIdg723yHXtwsKjl?p=preview

Unfortunately plunker with custom filter doesn't work with unminified version. it looks like it fails with dependency injection, but I have no idea why.

Uncaught Error: [$injector:modulerr] Failed to instantiate module mainApp due to: Error: [$injector:nomod] Module 'mainApp' is not available! You either misspelled the module name or forgot to load it. If registering a module ensure that you specify the dependencies as the second argument.

http://plnkr.co/edit/PsTzjVhpxQ0lkvXvysJV?p=preview

@PaulL1
Copy link
Contributor

PaulL1 commented Mar 14, 2015

That error is usually a typo in your plunker.

Thomas Koch added 2 commits March 16, 2015 16:14
I want to add a fourth custom template 'filterTemplate' and
don't want to copy the code a fourth time.
With this configuration setting it is possible to provide custom
filter templates for any filter.

Documentation and tests are still needed, but we first want to have
approval of the concept.
@pszawlis
Copy link

The plunker with custom filter works now in terms of display.

http://plnkr.co/edit/PsTzjVhpxQ0lkvXvysJV?p=preview

@sachin-bs
Copy link

Will this be a part of the next stable build as a stand alone feature?

@pszawlis
Copy link

I found the reason of the wrong filtering.

I made an error of referring to mainCtrl from headerCellTemplate (or filter directive in second solution). After moving to different controller and using $scope.grid.appScope everything works just fine.

Probably using broadcast would be better here, but it works

You can check two examples

headerCellTemplate
http://plnkr.co/edit/lcoTtIdg723yHXtwsKjl?p=preview

filter directive
http://plnkr.co/edit/PsTzjVhpxQ0lkvXvysJV?p=preview

PaulL1 added a commit to PaulL1/ng-grid that referenced this pull request Apr 12, 2015
Similar in concept to angular-ui#2918, but works with templates rather than directives.
Allows use of widgets other than the standard widgets, and provides a tutorial
on using the capability to produce custom dropdowns and the like.
@PaulL1
Copy link
Contributor

PaulL1 commented Apr 12, 2015

I've written something similar, using this code as an example, but using the concept of a template instead of a directive (semantics to some extent, but fits with our design patterns).

I've included a tutorial: http://ui-grid.info/docs/#/tutorial/306_custom_filters

Let me know if this meets your needs.

PaulL1 added a commit to PaulL1/ng-grid that referenced this pull request Apr 12, 2015
Similar in concept to angular-ui#2918, but works with templates rather than directives.
Allows use of widgets other than the standard widgets, and provides a tutorial
on using the capability to produce custom dropdowns and the like.
PaulL1 added a commit to PaulL1/ng-grid that referenced this pull request Apr 12, 2015
Similar in concept to angular-ui#2918, but works with templates rather than directives.
Allows use of widgets other than the standard widgets, and provides a tutorial
on using the capability to produce custom dropdowns and the like.
@PaulL1 PaulL1 closed this Apr 12, 2015
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

4 participants