-
-
Notifications
You must be signed in to change notification settings - Fork 73
Issue 370 - Clearing filtering_settings
clears the by-column filter UI
#372
Changes from all commits
ef11b66
5e08aed
7694077
8b7f2b3
c4054ee
f2e6ee4
14fe2e6
1580e99
6cb87b7
0df90ec
fe06dee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 => { | ||
|
@@ -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>); | ||
} | ||
} | ||
|
||
render() { | ||
return (<DataTable | ||
setProps={this.setProps} | ||
{...this.state.tableProps} | ||
/>); | ||
return (<div> | ||
{this.renderMode()} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', | ||
|
@@ -31,6 +32,11 @@ export const ReadWriteModes = [ | |
AppMode.Virtualized | ||
]; | ||
|
||
export const BasicModes = [ | ||
...ReadWriteModes, | ||
AppMode.ReadOnly | ||
]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||
|
@@ -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); | ||
|
||
|
@@ -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: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
}); | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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[]) => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,6 +64,10 @@ export default (lexerResult: ILexerResult): ISyntaxerResult => { | |
return { valid: false, error: `lexer -- ${lexerResult.error}` }; | ||
} | ||
|
||
if (lexerResult.lexemes.length === 0) { | ||
return { valid: true }; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
|
||
|
@@ -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 | ||
}); | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,8 +130,12 @@ export default class FilterFactory { | |
} | ||
|
||
const { tree } = syntaxerResult; | ||
const toCheck: (ISyntaxTree | undefined)[] = [tree]; | ||
if (!tree) { | ||
this.ops.clear(); | ||
return; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', '')); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And make sure the fields are empty -- as per described issue |
||
}); | ||
}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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