Conversation
| case AppMode.Default: | ||
| default: | ||
| return getDefaultState(); | ||
| } |
There was a problem hiding this comment.
Various pre-configured modes for the demo app to use
| } | ||
|
|
||
| function gendata(func, ndata = N_DATA) { | ||
| function gendata(func: (i: number) => any, ndata = N_DATA) { |
There was a problem hiding this comment.
JS to TS, adding typing info
| cy.get('.row-1').then($el => { | ||
| $el[0].style.overflow = toggled ? '' : 'unset'; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Cypress auto-scrolls very aggressively to guarantee element visibility -- this causes issues with the table's own scrolling mechanism and event hooks -- this allows disabling/enabling scrolling on the content section at will.
| DashTable.getDelete(0).click(); | ||
| DashTable.getCell(0, 0).within(() => cy.get('.dash-cell-value').should('have.html', '2')); | ||
| }); | ||
| Object.values(AppMode).forEach(mode => { |
There was a problem hiding this comment.
For all standalone tests, execute the same tests with all existing app modes
| @@ -0,0 +1,127 @@ | |||
| import DashTable from 'cypress/DashTable'; | |||
There was a problem hiding this comment.
This is a duplicate of the next file -- has been subsequently removed.
| @@ -0,0 +1,127 @@ | |||
| import DashTable from 'cypress/DashTable'; | |||
There was a problem hiding this comment.
This is a duplicate of the next file -- has been subsequently removed.
| "dash-table/*": ["./../../src/dash-table/*"] | ||
|
|
||
| "dash-table/*": ["./../../src/dash-table/*"], | ||
| "demo/*": ["./../../demo/*"] |
There was a problem hiding this comment.
Need access to AppMode
| viewport.data, | ||
| viewport.indices, | ||
| virtualized.data, | ||
| virtualized.indices, |
There was a problem hiding this comment.
Viewport is not the final result of the table processing its content anymore, now it's Virtualized!
data -> virtual_data -> viewport_data -> virtualized_data
If virtualization=false, virtualized_data === viewport_data + offset of zero
There was a problem hiding this comment.
The naming is not super clear to me, but I don't have any other suggestions, other than renaming everything, to something like data -> derived_data -> paginated_data -> virtualized_data (and even that perhaps doesn't make that much sense either). I think mostly the fact that the difference between virtual_data and virtualized_data isn't immediately obvious, and viewport_data to me kind of implies that that is what the virtualized_data should be. Just writing down some thoughts here, I guess.
| columns, | ||
| viewport.data, | ||
| virtualized.data, | ||
| virtualized.offset, |
There was a problem hiding this comment.
The offset is now required to reconcile the various memoized handlers, active and selected cell evaluations, etc.
| const DERIVED_REGEX = /^derived_/; | ||
|
|
||
| export default class Table extends Component<PropsWithDefaultsAndDerived> { | ||
| export default class Table extends Component<PropsWithDefaultsAndDerived, StandaloneState> { |
There was a problem hiding this comment.
Improving internal typing for standalone case by defining the State type -- the controlled table's state has been entirely moved back to the table now and modifies it through props.setState
This is necessary because we need to hook up to the scroll event and to eval various element sizes to make virtualization work correctly.
| virtual, | ||
| virtual_selected_rows | ||
| virtual_selected_rows, | ||
| virtualized |
There was a problem hiding this comment.
The ControlledTable's props are now a combination of all props and state (for standalone) + all derived props
There was a problem hiding this comment.
Note that virtualized_data is not part of the data contract with Dash -- this is purely internal
| /*#if DEV*/ | ||
| const props: any = this.props; | ||
| R.forEach( | ||
| key => props[key] === newProps[key] && Logger.fatal(`Updated prop ${key} was mutated`), |
There was a problem hiding this comment.
Some additional sanity here, make sure we are not sending a mutated version of an existing props property -- since some of the comps are pure, this prevents updates from happening correctly
| /*#if DEV*/ | ||
| const props: any = this.state; | ||
| R.forEach( | ||
| key => props[key] === (newProps as any)[key] && Logger.fatal(`Updated prop ${key} was mutated`), |
There was a problem hiding this comment.
Same as above.
| uiHeaders?: IUserInterfaceCell[]; | ||
| } | ||
|
|
||
| export type StandaloneState = IState & Partial<PropsWithDefaultsAndDerived>; |
There was a problem hiding this comment.
The table's standalone state is the combination of the connected props + basic state props
| import { memoizeOneFactory } from 'core/memoizer'; | ||
| import memoizerCache from 'core/memoizerCache'; | ||
| import { ICellFactoryOptions } from 'dash-table/components/Table/props'; | ||
| import { ICellFactoryProps } from 'dash-table/components/Table/props'; |
There was a problem hiding this comment.
Renaming, clean up
| (datum, rowIndex) => mapRow( | ||
| (column, columnIndex) => { | ||
| const active = isActiveCell(activeCell, rowIndex, columnIndex); | ||
| const active = isActiveCell(activeCell, rowIndex + offset.rows, columnIndex + offset.columns); |
There was a problem hiding this comment.
Now need the offset to reference the right row and column (although this one is always 0 for now)
| "width": 50, | ||
| "max_width": 50, | ||
| "min_width": 50 | ||
| } |
There was a problem hiding this comment.
Make width stable so the table doesn't resize itself constantly as we scroll!
|
Looks great! I found that for horizontal scrolling, I needed to "scroll twice". The first scoll nudged the headers like 5 pixels but didn't move anything else. Then, scrolling again moved the table. |
|
probably not blocking but there's some unexpected highlighting behaviour when copying a large selection (i can move this to a separate issue if we want to address after this pr/during clean up).
|
|
@chriddyp Hum.. looking at the code I think this is a pre-existing problem as the onScroll handling hasn't changed for that part -- I'll try to reproduce the issue. |
|
@chriddyp Tried in Chrome/FF/Safari with and without scroll "always" visible -- I can't reproduce -- if you scroll just a little does it eventually realign itself? The alignment delay is a known issue with fixed rows. I'm unable to tell from the gif exactly what/when this is happening. |
|
@cldougl Agreed. I would make this a subsequent PR to improve the behavior afterwards. |
…lization-2 # Conflicts: # CHANGELOG.md # dash_table/package-info.json # demo/App.js # demo/data.ts # package-lock.json # package.json # src/core/components/IsolatedInput/index.tsx # tests/cypress/tests/standalone/navigation_test.ts
valentijnnieman
left a comment
There was a problem hiding this comment.
Couple of questions / remarks. Looks good to me otherwise! I'll leave the approving up to others, when they're done with QA etc.
| } : (newProps: Partial<PropsWithDefaultsAndDerived>) => this.setState(newProps); | ||
| } : (newProps: Partial<PropsWithDefaultsAndDerived>) => { | ||
| /*#if DEV*/ | ||
| const props: any = this.state; |
There was a problem hiding this comment.
I find it slightly (but only slightly) confusing to assign state as props here. Just for clarity's sake, perhaps renaming it const state = this.state; is slightly more readable?
There was a problem hiding this comment.
This is what allows the table to operate on its own -- standalone. There's not much that can be done about it while keeping that capability.. setProps === setState in that scenario :)
There was a problem hiding this comment.
Oh misread the comment! I'm trying to be consistent and "play" with props in all cases even though the props really are state. If that makes sense..
| viewport.data, | ||
| viewport.indices, | ||
| virtualized.data, | ||
| virtualized.indices, |
There was a problem hiding this comment.
The naming is not super clear to me, but I don't have any other suggestions, other than renaming everything, to something like data -> derived_data -> paginated_data -> virtualized_data (and even that perhaps doesn't make that much sense either). I think mostly the fact that the difference between virtual_data and virtualized_data isn't immediately obvious, and viewport_data to me kind of implies that that is what the virtualized_data should be. Just writing down some thoughts here, I guess.
| return; | ||
| } | ||
|
|
||
| const { r1c1 } = this.refs as { [key: string]: HTMLElement }; |
There was a problem hiding this comment.
Could you explain the use of refs here? Is it absolutely necessary? Or is there perhaps a way to handle this more gracefully in React? I guess it's just to get the top cell, to do some calculations on, but I can see this be very costly on performance. If it needs to create a ref every time you scroll.
There was a problem hiding this comment.
I am unaware of using refs having (or not having) a performance cost. Can you elaborate?
r1c1 is actually the second's row's second's column's "cell", the table that contains the main content of the data table (or all the content without fixed rows/columns)
Without refs, I could reference the top $el of the component and do the querySelector on .row-1 .cell-1-1 tr > td:first-of-type not sure if this is better or worse. It's a little less React-ish than using refs to do the same thing but not significantly so I guess.
I'll move line 157 after the if.. might be evaluating it uselessly and running selectors is not free either!
There was a problem hiding this comment.
Also, while this is a very costly operation it happens only once -- once uiCell is set the if at line 151 will be triggered.
There was a problem hiding this comment.
I might have misread - if it happens only once then it's not so bad!
|
@valentijnnieman Did a small optimization change based on comments. As stated above, wondering how refs are expensive vs. other approaches? |
|
I can reproduce #253 (comment) but only in firefox and only when the first cell is selected
|
|
otherwise I'm not seeing any issues in the QA! |
|
@cldougl W-o-w! Thanks! I can reproduce the issue and changed the style so as the focused cell gets a box-shadow as normal instead of a border. |
…lization-2 # Conflicts: # CHANGELOG.md # dash_table/bundle.js # dash_table/demo.js # dash_table/package-info.json # package-lock.json # package.json # tests/cypress/tsconfig.json
cldougl
left a comment
There was a problem hiding this comment.
💃 everything is looking good for me now! I will open a new issue for #253 (comment) after this is merged
This PR closes #234 and introduces virtualization to the table.
To try it out
npm run build.watchhttp://localhost:8080/index.html?mode=virtualized
http://localhost:8080/index.html?mode=fixed,virtualized
or spin off the review app and access the
virtualizationappwill load a 26 columns by 100k lines dataset
This functionality change is big and complex and ties in to most features provided by the table. I will be very appreciative of anyone spending some time testing actual scenarios with it! :)
To activate add prop
virtualization=trueand recommend setting proppagination_mode=false(although virtualization and pagination can work together)This implementation of virtualization is based on some basic assumptions that we can revisited or lifted later
Other known limitations are:
The
virtualized_datais a new step in the chain of derived data generated by the table for display purposesdata: the entire dataset passed to the table
data + filtering + sorting -> virtual_data
virtual_data + pagination -> viewport_data
viewport_data + virtualization -> virtualized_data
Do note that the virtualized_data is not exposed to Dash as it is a considered a UI "implementation detail" -- in essence the user is still scrolling through the viewport, not in a special new state.
Most of the changes in this PR are to:
The first batch of commits below (bump version --> build artefacts) have been rebased and it can be helpful to look at them one at a time to isolate complexity. Afterwards there is multiple commits that have not been rebased in order to fix all remaining regressions. Haven't rebased the rebase as there is a lot less going on there anyway.
The table provides a default height/max-height when virtualization is toggled, same as for n_fixed_rows > 0 -- additional styling can be done with style_table. No
css: [...]overrides should be required.