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

For SELECT filter ng-options, use track by instead of selectAs #3408

Closed
wants to merge 3 commits into from

Conversation

ravishivt
Copy link
Contributor

This fixes the dropdown view when initializing the dropdown value’s "term" or when setting the "term" value programmatically such as during table restore state.

For an example of how this is a problem in ui-grid, please see this plnkr. For the plnkr, I've modified the Company field to be an object instead of a string and use the object to display multiple object properties in that column. A SELECT filter is in place and it works but say I wanted an initial filter so I set the term to be "{name: 'Oulu Incorporated', country: 'US'}". Although the table is properly filtered, the filter dropdown is blank. This is because Angular's ng-options determines that the term is not an identical reference to the option value. This problem is also encountered when doing a restoreState since again, ng-options will not detect the restored value to be the same as an existing option. Technically, we could fix this by adding a track by expression but AngularJS does not fully support "track by" and "selectAs" as per angular/angular.js#6564. Angular core team does not appear to be planning to fix this.

A workaround is described in that ticket, plnkr, which involves using "track by" instead of "selectAs" and then adjusting for the changes in the code. That workaround was used for this second plnkr and contains the fix in this pull request. Note that the initial value shows up in the dropdown, the table is properly filtered on that initial value, and that other dropdown options work as well.

Due to this change, the term that is set in ng-model now will be the full selectOption object with value and label properties instead of just the .value. This affects the comparison when the filters are run. Therefore, code was added to A) extract the .value property before doing the filter comparison and B) set up a default condition function for SELECT filters instead of using the default startswithRE regex. B helps when the selectOption .value is an object and not a string.

Review on Reviewable

This fixes the dropdown view when initializing the dropdown value’s
"term" or when setting the "term" value programmatically such as during
table restore state.
Since searchOption now uses "track by" instead of "selectAs", the
entire searchOption object will be set as the term.  This would require
a custom condition function for every SELECT filter since you would
need to extract the term.value before comparing to the cell value.
This code extracts the .value before it does the comparison.  It also
uses angular.equals() to compare the SELECT filters by default instead
of trying to use a starts with regex.  This solves the problem of using
objects instead of strings for searchOption values.
@JLLeitschuh
Copy link
Contributor

It would be nice if this came with a documentation change as this seems to add a new feature to filtering.
Also, it would be nice to see a unit test to prove that this is working correctly.
I tried your change against your example plunk and it didn't actually work: http://plnkr.co/edit/EoS5ou?p=preview

@ravishivt
Copy link
Contributor Author

The reason your plnkr fails is because the condition function for the company field is trying to use searchTerm.value. searchTerm is the object itself so there is no .value property. Therefore, you should use angular.equals(searchTerm, cellValue) or don't specify a condition function at all since that is the native comparison condition.

I wouldn't say this is a new feature. It is more making filtering to work as expected when the selectable filter options are objects and not simple strings.

I agree that a unit test would be helpful. I'll try to get around to that.

@mportuga
Copy link
Member

@ravishivt I still do not see any documentation changes or unit test changes. Closing for now. Feel free to open a new PR later if you so desire.

@mportuga mportuga closed this Sep 20, 2016
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.

None yet

3 participants