Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[DataTable] Adds ability to filter rows #1451

Open
wants to merge 9 commits into from

2 participants

@apipkin

Adds the ability to filter rows in the DataTable using the ModelList filter method in a non destructive way to the original data.

Multiple filters may be added, removed, and applied in any order. Filter functions are stored in a protected array to maintain order and filtered data is stored as a new internal ModelList. You can clear out the filters applied at any time and have the original data returned to the DataTable.

Filters can added as a single value, a range, a list of values, a regular expression to test, or a custom function. Filters are ran against each row in accordance to ModelList.filter method.

Ping @lsmith @rgrove @ericf

  • Code
  • Test
  • User Guides
  • User guides example
  • History
  • Sign Off or 3 days
@apipkin apipkin was assigned
@lsmith lsmith commented on the diff
src/datatable/docs/index.mustache
((111 lines not shown))
+ Y.one('#filter-clear').on('click', function (e) {
+ dt.clearFilters();
+ });
+
+ Y.one('#filter-name').on('click', function (e) {
+ dt.filter('name', /n/);
+ });
+
+ Y.one('#filter-qty').on('click', function (e) {
+ dt.filter('qty', 0, 20);
+ });
+
+```
+<div class="yui3-skin-sam dt-filter">
+ <div id="filter-dt"></div>
+ <br>
@lsmith
lsmith added a note

CSS, plz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith lsmith commented on the diff
src/datatable/js/filter.js
((267 lines not shown))
+ column formatter then this result will be filtered. This is good for
+ columns whose value is calculated rather than provided but can also be
+ effective if you want to match a formatted value. Based on the number of
+ rows and the complexity of the formatter this could significantly increase
+ the filter and rerender time so you may want to use this with caution.
+
+ @since @SINCE@
+ @protected
+ @method _addFilter
+ @param {String} colKey
+ @param {String, Number, Array, RegExp} match
+ @param {String, Number} [matchMax]
+ @param {Boolean} [useFormatter]
+ */
+ _addFilter: function (colKey, match, matchMax, useFormatter) {
+ var matchType = {}.toString.call(match),
@lsmith
lsmith added a note

matchType = Y.Lang.type(match) // 'array', 'regexp', etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith lsmith commented on the diff
src/datatable/js/filter.js
((273 lines not shown))
+ @since @SINCE@
+ @protected
+ @method _addFilter
+ @param {String} colKey
+ @param {String, Number, Array, RegExp} match
+ @param {String, Number} [matchMax]
+ @param {Boolean} [useFormatter]
+ */
+ _addFilter: function (colKey, match, matchMax, useFormatter) {
+ var matchType = {}.toString.call(match),
+ col,
+ formatterFn,
+ formatterData,
+ index = 0;
+
+ if (useFormatter) {
@lsmith
lsmith added a note

Was there specific request for this? I think filtering, like sorting, should be applied to the data, not to the visual representation of that data.

@apipkin
apipkin added a note

I noticed in testing that a simple currency formatter would round up values. That was why this was added as an option to turn on, and not an option to turn off

@lsmith
lsmith added a note

Meaning the column data would be something like 10.495, but the currency formatter would make that $10.50?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith lsmith commented on the diff
src/datatable/js/filter.js
((297 lines not shown))
+ formatterData = {
+ value : val,
+ data : row.toJSON(),
+ record : row,
+ rowIndex : index++,
+ column : col,
+ className : '',
+ rowClass : ''
+ };
+
+ if (formatterFn) {
+ val = formatterFn.call(this, formatterData);
+ }
+ }
+
+ if (typeof matchMax !== 'undefined' && matchMax !== null) {
@lsmith
lsmith added a note

An object config would be a better option.

table.addFilter({ key: 'foo', min: 0, max: 10 });
table.addFilter({ key: 'bar', value: 5 });
table.addFilter({ key: 'baz', values: [5, 10, 15] });
table.addFilter({ key: 'goop', match: /someRegex/ });
table.addFilter(function (rowData) { /* custom filter */ });
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith lsmith commented on the diff
src/datatable/js/filter.js
((321 lines not shown))
+ } else {
+ return val === match;
+ }
+ });
+ },
+
+ /**
+ Defines a getter for `ATTRS.data` where it will return the filtered or
+ unfiltered data.
+ @since @SINCE@
+ @protected
+ @method _alternativeDataGetter
+ @return {ModelList} ModelList containing filtered records or unchanged data
+ */
+ _alternativeDataGetter: function () {
+ return this.get('filteredData') || this.get('unmodifiedData');
@lsmith
lsmith added a note

Why not

_alternativeDataGetter: function (value) {
    return this._filters.length ? this.get('filteredData') : value;
}
@lsmith
lsmith added a note

or ... ? this._getStateVal('filteredData') : ... as you're doing below in _getUnmodifiedData.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith lsmith commented on the diff
src/datatable/js/filter.js
((49 lines not shown))
+@class DataTable.Filter
+
+**/
+function Filter() {}
+
+Filter.ATTRS = {
+
+ /**
+ Returns the original, unmodified data. This is the same as get('data') if
+ there are no filters applied.
+ @since @SINCE@
+ @attribute unmodifiedData
+ @type {ModelList}
+ @readonly
+ */
+ unmodifiedData: {
@lsmith
lsmith added a note

Why an attribute instead of providing access to the unfiltered data through a method?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith lsmith commented on the diff
src/datatable/js/filter.js
((61 lines not shown))
+ @type {ModelList}
+ @readonly
+ */
+ unmodifiedData: {
+ getter: '_getUnmodifiedData',
+ readOnly: true
+ },
+
+ /**
+ A ModelList of filtered data. If no filters are applied, this attribute is `null`.
+ @since @SINCE@
+ @attribute filteredData
+ @type {ModelList, null}
+ @readonly
+ */
+ filteredData: {
@lsmith
lsmith added a note

Similar question here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith lsmith commented on the diff
src/datatable/js/filter.js
((357 lines not shown))
+ */
+ _getFilteredData: function () {
+ return this._filteredData;
+ },
+
+ /**
+ Fires the `dataChange` event with a payload reflecting whether we are
+ filtering or are not filtering based on the existence of filtered data.
+ Then we fire the `dataChange` event to upated other components when the
+ data has been filtered or all the filters have been removed.
+
+ @since @SINCE@
+ @protected
+ @method _fireDataChange
+ */
+ _fireDataChange: function () {
@lsmith
lsmith added a note

This seems like an odd co-opting of Attribute's responsibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith lsmith commented on the diff
src/datatable/tests/manual/filter-drop.html
((192 lines not shown))
+<body>
+ <div id="pop"></div>
+ <div id="dt"></div>
+
+ <script>
+ YUI.add('popup-filter', function (Y, NAME) {
+ var HIDDEN = 'yui3-hidden',
+
+ PopupFilter = Y.Base.create('popup-filter', Y.Base, [Y.AutoCompleteBase], {
+ initializer: function () {
+ this._bindUIACBase();
+ this._syncUIACBase();
+ }
+ }),
+
+ PopupTemplate = '<h3 class="title">{{{title}}}</h3>\
@lsmith
lsmith added a note

Better to trust string concatenation than line continuation after an EOL \.

PopupTemplate =
    '<h3 class="title">{{{title}}}</h3>' +
    '<form class="colFilter pure-form">' +
    ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith lsmith commented on the diff
src/datatable/tests/manual/filter.html
@@ -0,0 +1,170 @@
+<!DOCTYPEhtml>
@lsmith
lsmith added a note

missing space

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith lsmith commented on the diff
src/datatable/tests/manual/filter.html
((35 lines not shown))
+<div id="table"></div>
+
+<div id="panel">
+<dl>
+ <dt>Key</dt><dd><input id="key"></dd>
+ <dt>Min</dt><dd><input id="min"></dd>
+ <dt>Max</dt><dd><input id="max"></dd>
+</dl>
+<label><input type="checkbox" id="useFn"> Use formatter function?</label>
+
+<button id="btn_filter">Filter</button>
+<button id="btn_clear">Clear</button>
+</div>
+
+
+<script type="text/javascript">
@lsmith
lsmith added a note

type="text/javascript"? I'm guessing this is copied from an old file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith lsmith commented on the diff
...datatable/tests/unit/assets/datatable-filter-tests.js
@@ -0,0 +1,246 @@
+YUI.add('datatable-filter-tests', function(Y) {
+
+var suite = new Y.Test.Suite("DataTable: filter");
+
+var dt = new Y.DataTable({
@lsmith
lsmith added a note

This should be in the setUp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith lsmith commented on the diff
...datatable/tests/unit/assets/datatable-filter-tests.js
((17 lines not shown))
+ { name: 'Orange', qty: 3 },
+ { name: 'Pineapple', qty: 2 }
+ ],
+ render: true
+ }),
+
+ areSame = Y.Assert.areSame;
+
+
+
+suite.add(new Y.Test.Case({
+
+ name: "filter",
+
+ 'setUp': function () {
+ dt.clearFilters();
@lsmith
lsmith added a note

It's fine to share existing DOM between tests so long as you don't change it. But don't share objects between tests. The chance of contamination is too high. Create a new DT for each test, and destroy it in tearDown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith

Is there a design history for this feature? Or can you share your thoughts on why:

  • filter(...) doesn't replace all filters with the input
  • there isn't a filters attribute to expose the array of filters to the implementer
  • it's necessary to manually fire the dataChange event
  • filter supports so many signatures (and includes a boolean trap) rather than taking an object config/function per filter
  • the useFormatter flag is supported
@lsmith

Incomplete thoughts:

var table = new Y.DataTable({
    data: [ ... ],
    columns: [ ... ],
    // support instantiation-time config of filters
    filters: [ { key: 'foo', value: 100 }, function (rowData) { return rowData.bar > 100; } ]
});

table.set('filters', null); // or [] to clear all filters. This could be the implementation of clearFilters()
table.set('filters', [{ key: 'foo', min: 0, max: 100 }]); // replace all filters. This is the implementation of filter(...)
table.filter({ key: 'foo', match: /blah/ }); // replaces all filters
table.addFilters([{ key: 'foo', values: [5, 10, 15, 100] }]); // appends the filters attr array, method name is plural, expects an array

table.filter(..., { silent: true }); // Don't immediately reflow
table.addFilter(..., { silent: true }); // same
table.clearFilters({ silent: true }); // same

table.filter({ key: 'foo', value: 'single column filter' });
table.filter({ name: 'foo', value: 'maybe support "name" as well or instead of "key"?' });
table.filter({ col: 'foo', value: 'or really mess with @Satyam and introduce a new noun ;)' }); // or 'column'

table.filter([ { key: 'foo', values: ['pass array', 'to filter()'] }, { key: 'bar', value: 'to replace all filters with a new set' } ] });
table.filter(function (rowData) { /* boolean function */ }); // obviates { key: 'foo', fn: function ()... }

Store the filters as they were passed to filter(), addFilter(), or set('filters', [HERE]), and do type checking of filters in the code that will return the filtered data. Exception would be if an array is passed, its contents are appended to the end of the array stored in 'filters'.

The change event for filters should trigger a syncUI or this.fire('renderTable') or something like that. Hmm, communicating the change specifically to the body view would violate decoupling. Is that why you were firing the dataChange?

@apipkin apipkin removed their assignment
@apipkin apipkin was assigned by okuryu
@apipkin apipkin was unassigned by okuryu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.