Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

Issue 370 - Clearing filtering_settings clears the by-column filter UI #372

Merged
merged 11 commits into from
Feb 19, 2019
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- `style_header_conditional`

### Fixed
[#347](https://github.com/plotly/dash-core/issues/347)
- Fixed table behavior when `filtering_settings` is updated through a callback

[#322](https://github.com/plotly/dash-core/issues/322)
- Added LICENSE file to Python distributable

Expand Down
32 changes: 26 additions & 6 deletions demo/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,18 @@
import * as R from 'ramda';
import React, {Component} from 'react';
import { DataTable } from 'dash-table';
import Environment from 'core/environment';
import { memoizeOne } from 'core/memoizer';
import Logger from 'core/Logger';
import AppMode from './AppMode';
import AppState, { AppMode } from './AppMode';

import './style.less';

class App extends Component {
constructor() {
super();

this.state = AppMode;
this.state = AppState;

const setProps = memoizeOne(() => {
return newProps => {
Expand All @@ -28,11 +29,30 @@ class App extends Component {
});
}

renderMode() {
const mode = Environment.searchParams.get('mode');

if (mode === AppMode.Filtering) {
return (<button
className='clear-filters'
onClick={() => {
const tableProps = R.clone(this.state.tableProps);
tableProps.filtering_settings = '';

this.setState({ tableProps });
}}
>Clear Filter</button>);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this new case, need an additional button to trigger the filter reset -- either that or make this a full on e2e test, which I think is pointless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mode based conditional additional comps


render() {
return (<DataTable
setProps={this.setProps}
{...this.state.tableProps}
/>);
return (<div>
{this.renderMode()}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conditionals added with the table

<DataTable
setProps={this.setProps}
{...this.state.tableProps}
/>
</div>);
}
}

Expand Down
15 changes: 15 additions & 0 deletions demo/AppMode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { TooltipSyntax } from 'dash-table/tooltips/props';
export enum AppMode {
Date = 'date',
Default = 'default',
Filtering = 'filtering',
FixedTooltips = 'fixed,tooltips',
FixedVirtualized = 'fixed,virtualized',
ReadOnly = 'readonly',
Expand All @@ -31,6 +32,11 @@ export const ReadWriteModes = [
AppMode.Virtualized
];

export const BasicModes = [
...ReadWriteModes,
AppMode.ReadOnly
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each time we added a mode we were running the standalone tests against an additional mode. In most cases, we want a new mode to test specific aspects of the table not all cases.


function getBaseTableProps(mock: IDataMock) {
return {
id: 'table',
Expand Down Expand Up @@ -209,6 +215,13 @@ function getDateState() {
return state;
}

function getFilteringState() {
const state = getDefaultState();
state.tableProps.filtering = true;

return state;
}

function getVirtualizedState() {
const mock = generateMockData(5000);

Expand Down Expand Up @@ -253,6 +266,8 @@ function getState() {
switch (mode) {
case AppMode.Date:
return getDateState();
case AppMode.Filtering:
return getFilteringState();
case AppMode.FixedTooltips:
return getFixedTooltipsState();
case AppMode.FixedVirtualized:
Expand Down
10 changes: 10 additions & 0 deletions src/core/components/IsolatedInput/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ export default class IsolatedInput extends PureComponent<IProps, IState> {
return this.props as PropsWithDefaults;
}

componentWillReceiveProps(nextProps: IProps) {
const { value: nextValue } = nextProps;

if (this.state.value !== nextValue) {
this.setState({
value: nextValue
});
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The input needs to update its internal state when the value/prop updates. That prevented the UI from updating to match the filter.

constructor(props: PropsWithDefaults) {
super(props);

Expand Down
10 changes: 4 additions & 6 deletions src/core/syntax-tree/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,16 @@ export default class SyntaxTree {
}

evaluate = (target: any) => {
if (!this.isValid || !this.tree) {
if (!this.isValid) {
const msg = `unable to evaluate target: syntax tree is invalid for query=${this.query}`;

Logger.error(msg);
throw new Error(msg);
}

const evaluate = this.tree.lexeme.evaluate;

return evaluate ?
evaluate(target, this.tree) :
false;
return this.tree && this.tree.lexeme && this.tree.lexeme.evaluate ?
this.tree.lexeme.evaluate(target, this.tree) :
true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A valid null tree (no query string) now evaluates to true (the target matches the criteria)

}

filter = (targets: any[]) => {
Expand Down
4 changes: 4 additions & 0 deletions src/core/syntax-tree/syntaxer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ export default (lexerResult: ILexerResult): ISyntaxerResult => {
return { valid: false, error: `lexer -- ${lexerResult.error}` };
}

if (lexerResult.lexemes.length === 0) {
return { valid: true };
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having no query string is now a valid / null tree case instead of an invalid case


try {
return { tree: parser(lexemes), valid: true };
} catch (error) {
Expand Down
16 changes: 1 addition & 15 deletions src/dash-table/components/Filter/Column.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,7 @@ interface IColumnFilterProps {
value?: string;
}

interface IColumnFilterState {
value?: string;
}

export default class ColumnFilter extends PureComponent<IColumnFilterProps, IColumnFilterState> {
export default class ColumnFilter extends PureComponent<IColumnFilterProps> {
constructor(props: IColumnFilterProps) {
super(props);

Expand All @@ -27,16 +23,6 @@ export default class ColumnFilter extends PureComponent<IColumnFilterProps, ICol
};
}

componentWillReceiveProps(nextProps: IColumnFilterProps) {
const { value: nextValue } = nextProps;

if (this.state.value !== nextValue) {
this.setState({
value: nextValue
});
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was dead code -- dating back to before the IsolatedInput was added - nothing depends on the state here, removing.

private submit = (value: string | undefined) => {
const { setFilter } = this.props;

Expand Down
6 changes: 5 additions & 1 deletion src/dash-table/components/FilterFactory.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,12 @@ export default class FilterFactory {
}

const { tree } = syntaxerResult;
const toCheck: (ISyntaxTree | undefined)[] = [tree];
if (!tree) {
this.ops.clear();
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When rebuilding the fragments, clear all fragments if there's no query


const toCheck: (ISyntaxTree | undefined)[] = [tree];
while (toCheck.length) {
const item = toCheck.pop();
if (!item) {
Expand Down
81 changes: 63 additions & 18 deletions tests/cypress/tests/standalone/filtering_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,31 +5,76 @@ import Key from 'cypress/Key';
import { AppMode } from 'demo/AppMode';

describe(`filter`, () => {
beforeEach(() => {
cy.visit(`http://localhost:8080?mode=${AppMode.ColumnsInSpace}`);
DashTable.toggleScroll(false);
describe(`special characters`, () => {
beforeEach(() => {
cy.visit(`http://localhost:8080?mode=${AppMode.ColumnsInSpace}`);
DashTable.toggleScroll(false);
});

it('can filter on special column id', () => {
DashTable.getFilterById('c cc').click();
DOM.focused.type(`gt num(90)${Key.Enter}`);

DashTable.getFilterById('d:dd').click();
DOM.focused.type(`lt num(12500)${Key.Enter}`);

DashTable.getFilterById('e-ee').click();
DOM.focused.type(`is prime${Key.Enter}`);

DashTable.getFilterById('f_ff').click();
DOM.focused.type(`le num(106)${Key.Enter}`);

DashTable.getFilterById('g.gg').click();
DOM.focused.type(`gt num(1000)${Key.Enter}`);

DashTable.getFilterById('b+bb').click();
DOM.focused.type(`eq "Wet"${Key.Enter}`);

DashTable.getCellById(0, 'rows').within(() => cy.get('.dash-cell-value').should('have.html', '101'));
DashTable.getCellById(1, 'rows').should('not.exist');
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird diff here but nothing changed

});

it('can filter on special column id', () => {
DashTable.getFilterById('c cc').click();
DOM.focused.type(`gt num(90)${Key.Enter}`);
describe('reset', () => {
beforeEach(() => {
cy.visit(`http://localhost:8080?mode=${AppMode.Filtering}`);
DashTable.toggleScroll(false);
});

it('updates results and filter fields', () => {
let cell_0;
let cell_1;

DashTable.getCellById(0, 'ccc')
.within(() => cy.get('.dash-cell-value')
.then($el => cell_0 = $el[0].innerHTML));

DashTable.getFilterById('d:dd').click();
DOM.focused.type(`lt num(12500)${Key.Enter}`);
DashTable.getCellById(1, 'ccc')
.within(() => cy.get('.dash-cell-value')
.then($el => cell_1 = $el[0].innerHTML));

DashTable.getFilterById('e-ee').click();
DOM.focused.type(`is prime${Key.Enter}`);
DashTable.getFilterById('ccc').click();
DOM.focused.type(`gt num(100)`);
DashTable.getFilterById('ddd').click();
DOM.focused.type('lt num(20000)');
DashTable.getFilterById('eee').click();
DOM.focused.type('is prime');
DashTable.getFilterById('bbb').click();
DOM.focused.type(`eq "Wet"`);
DashTable.getFilterById('ccc').click();

DashTable.getFilterById('f_ff').click();
DOM.focused.type(`le num(106)${Key.Enter}`);
DashTable.getCellById(0, 'ccc').within(() => cy.get('.dash-cell-value').should('have.html', '101'));
DashTable.getCellById(1, 'ccc').within(() => cy.get('.dash-cell-value').should('have.html', '109'));

DashTable.getFilterById('g.gg').click();
DOM.focused.type(`gt num(1000)${Key.Enter}`);
cy.get('.clear-filters').click();

DashTable.getFilterById('b+bb').click();
DOM.focused.type(`eq "Wet"${Key.Enter}`);
DashTable.getCellById(0, 'ccc').within(() => cy.get('.dash-cell-value').should('have.html', cell_0));
DashTable.getCellById(1, 'ccc').within(() => cy.get('.dash-cell-value').should('have.html', cell_1));

DashTable.getCellById(0, 'rows').within(() => cy.get('.dash-cell-value').should('have.html', '101'));
DashTable.getCellById(1, 'rows').should('not.exist');
DashTable.getFilterById('bbb').within(() => cy.get('input').should('have.value', ''));
DashTable.getFilterById('ccc').within(() => cy.get('input').should('have.value', ''));
DashTable.getFilterById('ddd').within(() => cy.get('input').should('have.value', ''));
DashTable.getFilterById('eee').within(() => cy.get('input').should('have.value', ''));
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apply a filter column by column, see that it's applied, clear it, see that it's not applied anymore!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And make sure the fields are empty -- as per described issue

});
});
4 changes: 2 additions & 2 deletions tests/cypress/tests/standalone/navigation_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import DashTable from 'cypress/DashTable';
import DOM from 'cypress/DOM';
import Key from 'cypress/Key';

import { AppMode } from 'demo/AppMode';
import { BasicModes } from 'demo/AppMode';

Object.values(AppMode).forEach(mode => {
Object.values(BasicModes).forEach(mode => {
describe(`navigate, mode=${mode}`, () => {
beforeEach(() => {
cy.visit(`http://localhost:8080?mode=${mode}`);
Expand Down
4 changes: 2 additions & 2 deletions tests/cypress/tests/standalone/select_row_test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import DashTable from 'cypress/DashTable';

import { AppMode } from 'demo/AppMode';
import { BasicModes } from 'demo/AppMode';

Object.values(AppMode).forEach(mode => {
Object.values(BasicModes).forEach(mode => {
describe(`select row, mode=${mode}`, () => {
beforeEach(() => {
cy.visit(`http://localhost:8080?mode=${mode}`);
Expand Down
4 changes: 2 additions & 2 deletions tests/cypress/tests/standalone/selection_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import DashTable from 'cypress/DashTable';
import DOM from 'cypress/DOM';
import Key from 'cypress/Key';

import { AppMode } from 'demo/AppMode';
import { BasicModes } from 'demo/AppMode';

Object.values(AppMode).forEach(mode => {
Object.values(BasicModes).forEach(mode => {
describe(`select, mode=${mode}`, () => {
beforeEach(() => {
cy.visit(`http://localhost:8080?mode=${mode}`);
Expand Down