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

Regression fix multi table typing #468

Merged
merged 7 commits into from
Jun 17, 2019
Merged

Regression fix multi table typing #468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b579f3d
isolate shared memoized calcs
Jun 13, 2019
46b8824
- add cell edi test with two tables
Jun 14, 2019
c28bf81
update test
Jun 14, 2019
4db77d1
fix sanitation memoize
Jun 14, 2019
5cd918e
Merge branch 'master' into regression-fix-multi-table-typing
Marc-Andre-Rivet Jun 14, 2019
87e3f38
add explanation for weird test setup
Jun 17, 2019
04ae9c2
Merge branch 'regression-fix-multi-table-typing' of github.com:plotly…
Jun 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 0 additions & 75 deletions demo/App.js
View file
Open in desktop

This file was deleted.

84 changes: 84 additions & 0 deletions demo/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/* eslint no-magic-numbers: 0 */
import * as R from 'ramda';
import React, {Component} from 'react';
import { DataTable } from 'dash-table/index';
import Environment from 'core/environment';
import { memoizeOne } from 'core/memoizer';
import Logger from 'core/Logger';
import AppState, { AppMode } from './AppMode';
import memoizerCache from 'core/cache/memoizer';

import './style.less';

class App extends Component<any, any> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming to .tsx file but mostly just filling in with any. Can improve some more later. At least now it's TS too.

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

this.state = {
...AppState,
temp_filtering: ''
};
}

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

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

this.setState({ tableProps });
}}
>Clear Filter</button>
<input
style={{ width: '500px' }}
value={this.state.temp_filtering}
onChange={
e => this.setState({ temp_filtering: e.target.value })
}
onBlur={e => {
const tableProps = R.clone(this.state.tableProps);
tableProps.filter_query = e.target.value;

this.setState({ tableProps });
}} />
</div>);
} else if (mode === AppMode.TaleOfTwoTables) {
const props: any = {};
Object.entries(this.state.tableProps).forEach(([key, value]) => {
props[key] = this.propCache.get(key)(value);
});

return (<DataTable
{...props}
/>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two tables mode: shallow copy each prop & memoize -- feed the 2nd table with these new values.

}
}

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

private propCache = memoizerCache<[string]>()(R.clone);

private setProps = memoizeOne(() => {
return (newProps: any) => {
Logger.debug('--->', newProps);
this.setState((prevState: any) => ({
tableProps: R.merge(prevState.tableProps, newProps)
}));
};
});
}

export default App;
2 changes: 2 additions & 0 deletions demo/AppMode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export enum AppMode {
Formatting = 'formatting',
ReadOnly = 'readonly',
ColumnsInSpace = 'columnsInSpace',
TaleOfTwoTables = 'taleOfTwoTables',
Tooltips = 'tooltips',
Typed = 'typed',
Virtualized = 'virtualized'
Expand Down Expand Up @@ -335,6 +336,7 @@ function getState() {
return getVirtualizedState();
case AppMode.Typed:
return getTypedState();
case AppMode.TaleOfTwoTables:
case AppMode.Default:
default:
return getDefaultState();
Expand Down
5 changes: 3 additions & 2 deletions src/dash-table/dash/DataTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,22 @@ import Logger from 'core/Logger';

import genRandomId from 'dash-table/utils/generate';
import isValidProps from './validate';
import sanitizeProps from './sanitize';
import Sanitizer from './Sanitizer';

export default class DataTable extends Component {
constructor(props) {
super(props);
let id;
this.getId = () => (id = id || genRandomId('table-'));
this.sanitizer = new Sanitizer();
}

render() {
if (!isValidProps(this.props)) {
return (<div>Invalid props combination</div>);
}

const sanitizedProps = sanitizeProps(this.props);
const sanitizedProps = this.sanitizer.sanitize(this.props);
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 table instance has its sanitizer instance

return this.props.id ?
(<RealTable {...sanitizedProps} />) :
(<RealTable {...sanitizedProps} id={this.getId()} />);
Expand Down
56 changes: 30 additions & 26 deletions src/dash-table/dash/sanitize.ts → src/dash-table/dash/Sanitizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,23 +29,6 @@ const D3_DEFAULT_LOCALE: INumberLocale = {
const DEFAULT_NULLY = '';
const DEFAULT_SPECIFIER = '';

const applyDefaultToLocale = memoizeOne((locale: INumberLocale) => getLocale(locale));

const applyDefaultsToColumns = memoizeOne(
(defaultLocale: INumberLocale, defaultSort: SortAsNull, columns: Columns, editable: boolean ) => R.map(column => {
const c = R.clone(column);
c.editable = isEditable(editable, column.editable);
c.sort_as_null = c.sort_as_null || defaultSort;

if (c.type === ColumnType.Numeric && c.format) {
c.format.locale = getLocale(defaultLocale, c.format.locale);
c.format.nully = getNully(c.format.nully);
c.format.specifier = getSpecifier(c.format.specifier);
}
return c;
}, columns)
);

const data2number = (data?: any) => +data || 0;

const getFixedColumns = (
Expand All @@ -64,16 +47,37 @@ const getFixedRows = (
0 :
headerRows(columns) + (filter_action !== TableAction.None ? 1 : 0) + data2number(fixed.data);

export default (props: PropsWithDefaults): SanitizedProps => {
const locale_format = applyDefaultToLocale(props.locale_format);
const applyDefaultsToColumns = (defaultLocale: INumberLocale, defaultSort: SortAsNull, columns: Columns, editable: boolean) => R.map(column => {
const c = R.clone(column);
c.editable = isEditable(editable, column.editable);
c.sort_as_null = c.sort_as_null || defaultSort;

return R.merge(props, {
columns: applyDefaultsToColumns(locale_format, props.sort_as_null, props.columns, props.editable),
fixed_columns: getFixedColumns(props.fixed_columns, props.row_deletable, props.row_selectable),
fixed_rows: getFixedRows(props.fixed_rows, props.columns, props.filter_action),
locale_format
});
};
if (c.type === ColumnType.Numeric && c.format) {
c.format.locale = getLocale(defaultLocale, c.format.locale);
c.format.nully = getNully(c.format.nully);
c.format.specifier = getSpecifier(c.format.specifier);
}
return c;
}, columns);

const applyDefaultToLocale = (locale: INumberLocale) => getLocale(locale);

export default class Sanitizer {
sanitize(props: PropsWithDefaults): SanitizedProps {
const locale_format = this.applyDefaultToLocale(props.locale_format);

return R.merge(props, {
columns: this.applyDefaultsToColumns(locale_format, props.sort_as_null, props.columns, props.editable),
fixed_columns: getFixedColumns(props.fixed_columns, props.row_deletable, props.row_selectable),
fixed_rows: getFixedRows(props.fixed_rows, props.columns, props.filter_action),
locale_format
});
}

private readonly applyDefaultToLocale = memoizeOne(applyDefaultToLocale);

private readonly applyDefaultsToColumns = memoizeOne(applyDefaultsToColumns);
}

export const getLocale = (...locales: Partial<INumberLocale>[]): INumberLocale =>
R.mergeAll([
Expand Down
51 changes: 37 additions & 14 deletions tests/cypress/tests/standalone/edit_cell_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,43 @@ import Key from 'cypress/Key';

import { AppMode, ReadWriteModes } from 'demo/AppMode';

Object.values([
...ReadWriteModes,
AppMode.TaleOfTwoTables
]).forEach(mode => {
describe(`edit, mode=${mode}`, () => {
beforeEach(() => {
cy.visit(`http://localhost:8080?mode=${mode}`);
DashTable.toggleScroll(false);
});

// Case: "Pressing enter to confirm change does not work on the last row"
// Issue: https://github.com/plotly/dash-table/issues/50
it('can edit on "enter"', () => {
DashTable.getCell(0, 1).click();
// Case: 2+ tables results in infinite rendering loop b/c of shared cache
// Issue: https://github.com/plotly/dash-table/pull/468
//
// Artificial delay added to make sure re-rendering has time to occur.
cy.wait(1000);
DOM.focused.type(`abc${Key.Enter}`);
DashTable.getCell(0, 1).within(() => cy.get('.dash-cell-value').should('have.html', `abc`));
});

it('can edit when clicking outside of cell', () => {
DashTable.getCell(0, 1).click();
DOM.focused.type(`abc`);
// Case: 2+ tables results in infinite rendering loop b/c of shared cache
// Issue: https://github.com/plotly/dash-table/pull/468
//
// Artificial delay added to make sure re-rendering has time to occur.
cy.wait(1000);
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 delay is not great but need to guarantee the table has had time to re-render if stuck in a loop.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm OK. I guess if it's an async system causing the infinite loop that makes sense. Please put a comment by each of these waits referencing this PR so we know why it's there and we don't try to optimize it away later, since presumably the test will still pass if it were removed - it just wouldn't be testing what it's supposed to be testing.

DashTable.getCell(0, 0).click();
DashTable.getCell(0, 1).within(() => cy.get('.dash-cell-value').should('have.html', `abc`));
});
});
});

Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Jun 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Subset of edit tests that are known to be impacted by the presence of a second table.

Pulled out from the tests below and applied to one more flavor.

Object.values(ReadWriteModes).forEach(mode => {
describe(`edit, mode=${mode}`, () => {
beforeEach(() => {
Expand Down Expand Up @@ -106,20 +143,6 @@ Object.values(ReadWriteModes).forEach(mode => {
cy.get('.Select-value-label').should('have.html', expectedValue);
});
});

// https://github.com/plotly/dash-table/issues/50
it('can edit on "enter"', () => {
DashTable.getCell(0, 1).click();
DOM.focused.type(`abc${Key.Enter}`);
DashTable.getCell(0, 1).within(() => cy.get('.dash-cell-value').should('have.html', `abc`));
});

it('can edit when clicking outside of cell', () => {
DashTable.getCell(0, 1).click();
DOM.focused.type(`abc`);
DashTable.getCell(0, 0).click();
DashTable.getCell(0, 1).within(() => cy.get('.dash-cell-value').should('have.html', `abc`));
});
});
});

Expand Down
2 changes: 1 addition & 1 deletion tests/cypress/tests/unit/formatting_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getFormatter } from 'dash-table/type/number';
import { getLocale, getNully, getSpecifier } from 'dash-table/dash/sanitize';
import { getLocale, getNully, getSpecifier } from 'dash-table/dash/Sanitizer';

describe('formatting', () => {
describe('number', () => {
Expand Down