Skip to content

Stop filter from matching invisible properties#800

Merged
dahlbyk merged 1 commit into
GriddleGriddle:masterfrom
mbland:filterable-#725
Mar 5, 2018
Merged

Stop filter from matching invisible properties#800
dahlbyk merged 1 commit into
GriddleGriddle:masterfrom
mbland:filterable-#725

Conversation

@mbland

@mbland mbland commented Mar 3, 2018

Copy link
Copy Markdown
Contributor

Griddle major version

1

Changes proposed in this pull request

Closes #725. This fixes the problem of invisible fields getting matched by the filter by updating the filteredDataSelector() condition to exclude griddleKey and any values not covered by a ColumnDefinition if any ColumnDefinitions are present.

Bear in mind that columns corresponding to other React components may still match the filter based on generated HTML properties rendered as part of the value.toString().toLowerCase() expression. At this point the best course of action may be to recommend that such ColumnDefinitions are tagged with filterable={false} if it's a significant problem for the user.

Why these changes are made

The previous if (filterable === false) condition meant that filterable={false} must be explicitly set on a ColumnDefinition in order for any property returned by row.keySeq() to be ignored. This resulted in the filter matching invisible row properties such as griddleKey and any properties in the data not covered by a ColumnDefinition.

I confirmed this by locally changing the expression at the end of this filter from:

  return value &&
    value.toString().toLowerCase().indexOf(filterToLower) > -1;

to:

  if (value && value.toString().toLowerCase().indexOf(filterToLower) > -1) {
    console.log('KEY:', key, 'VALUE:', value.toString().toLowerCase());
    return true;
  }
  return false;

and observed the unexpected filter matches on invisible row properties in the console.

Are there tests?

I've added updated existing tests, added new tests, and validated the filter behavior in the Storybook.

Closes GriddleGriddle#725. The previous `if (filterable === false)` condition meant
that `filterable={false}` must be explicitly set on a ColumnDefinition
in order for any property returned by row.keySeq() to be ignored. This
resulted in the filter matching invisible row properties such as
griddleKey and any properties in the data not covered by a
ColumnDefinition.

I confirmed this by locally changing the expression at the end of this
filter from:

  return value &&
    value.toString().toLowerCase().indexOf(filterToLower) > -1;

to:

  if (value && value.toString().toLowerCase().indexOf(filterToLower) > -1) {
    console.log('KEY:', key, 'VALUE:', value.toString().toLowerCase());
    return true;
  }
  return false;

and observed the unexpected filter matches on invisible row properties
in the console.

This fixes the problem by updating the condition to exclude griddleKey
and any values not covered by a ColumnDefinition if any
ColumnDefinitions are present. I've added new tests and validated the
filter behavior in the Storybook.

Bear in mind that columns corresponding to other React components may
still match the filter based on generated HTML properties rendered as
part of the value.toString().toLowerCase() expression. At this point the
best course of action may be to recommend that such ColumnDefinitions
are tagged with `filterable={false}` if it's a significant problem for
the user.
@dahlbyk

dahlbyk commented Mar 5, 2018

Copy link
Copy Markdown
Contributor

💯

@dahlbyk dahlbyk merged commit eab0dff into GriddleGriddle:master Mar 5, 2018
@mbland mbland deleted the filterable-#725 branch March 5, 2018 15:27
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.

how to stop filtering on not visible field?

2 participants