This repository has been archived by the owner on Jun 4, 2024. It is now read-only.
3.1 issue118 width behavior #130
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 30 commits
September 27, 2018 11:12
- separating operation cells from content
…-rendering # Conflicts: # dash_table/bundle.js # dash_table/demo.js
- separate wrappers from styles
- fix rendering perf regression
| {...attributes} | ||
| />); | ||
| (<div className='dash-input-cell-value-container dash-cell-value-container'> | ||
| {this.renderValue({ className: 'input-cell-value-shadow cell-value-shadow' })} |
There was a problem hiding this comment.
shadow div vor column size
There was a problem hiding this comment.
Could you explain this shadow div stuff for me in more detail? Looks interesting, but not sure why it is needed.
There was a problem hiding this comment.
When not applying a width to the columns, the columns are using the width of the content to decide what size they need to be. Since inputs force the width to be a lot bigger (inputs don't exactly respect normal width/sizing flow) than necessary (~150px) they create this undesirable effect of making columns bigger when a text|number cell is selected. That happens because text|number cells don't actually contain an input element when they are not selected, they just contain a div that shows the value (having thousands of inputs in a page causes significant slowdowns for all webkit based browsers, possibly others too) -- so.. since the input is alone in the table it's capable of forcing columns into a different size..
To prevent that, the input is made to be absolutely positioned and to fill the cell. Since the selected cell might be the one causing the column to have its current width, something has to force the column width when that input is present instead of the div.. hence the shadow div with the same style as the other normal div + some stuff to make it essentially invisible / have no impact on the render for the user.
There was a problem hiding this comment.
I don't find this solution awesome to be honest but short of figuring out (1) another way for inputs to respect the width (or have decent perf with 1k+ inputs) and (2) force dropdowns to take the actual amount of space they are supposed to do (hint: there's absolute positioning the Select implementation, so that's not really for us to decide), I don't see a better way to do this for now.
| const classes = [ | ||
| 'dash-spreadsheet-inner', | ||
| 'dash-spreadsheet', | ||
| ...(n_fixed_rows ? ['dash-freeze-top'] : []), | ||
| ...(n_fixed_columns ? ['dash-freeze-left'] : []) | ||
| ...(n_fixed_columns ? ['dash-freeze-left'] : []), | ||
| [`dash-${content_style}`] |
There was a problem hiding this comment.
marker class for css / flex styling
| table { | ||
| width: 100%; | ||
| } | ||
| } |
There was a problem hiding this comment.
content_style='grow' fill space available
| .dash-cell-value { | ||
| height: 100%; | ||
| width: 100%; | ||
| } | ||
|
|
||
| input.dash-cell-value { |
There was a problem hiding this comment.
for both focused cells with text|number type (with input) and filter cells, absolutely position the input and fill space.. this way it does not affect the size of the cell with default width
There was a problem hiding this comment.
Will need to make sure this works well with all custom styling scenarios when we revisit it
| } | ||
|
|
||
| th.dash-filter { | ||
| position: relative; |
There was a problem hiding this comment.
as above for cell inputs
| @@ -280,13 +280,137 @@ def layout(): | |||
| section_title("Dash Table - Padding"), | |||
| # ... | |||
| section_title("Dash Table - All Column Widths by Percent"), | |||
| # ... | |||
| html.Div( | |||
There was a problem hiding this comment.
@valentijnnieman I might be duplicating some of your previous work here? My apology if that's the case.
There was a problem hiding this comment.
No worries, as long as all the examples are there :) (looks to be the case, except for the others that aren't merged yet I guess?)
| }; | ||
|
|
||
| const props = Object.assign({}, baseProps, { | ||
| columns: columns.map((id => ({ id: id, name: id.toUpperCase(), width: 20, minWidth: 20, maxWidth: 20 }))) |
There was a problem hiding this comment.
test case with all width/minwidth/maxwidth set
| }; | ||
|
|
||
| const props = Object.assign({}, baseProps, { | ||
| columns: columns.map((id => ({ id: id, name: id.toUpperCase() }))) |
There was a problem hiding this comment.
test cases with default width
| }; | ||
|
|
||
| const props = Object.assign({}, baseProps, { | ||
| columns: columns.map((id => ({ id: id, name: id.toUpperCase(), maxWidth: 10 }))) |
There was a problem hiding this comment.
test cases with default width + a maximum
| }; | ||
|
|
||
| const props = Object.assign({}, baseProps, { | ||
| columns: columns.map((id => ({ id: id, name: id.toUpperCase(), minWidth: 100 }))) |
There was a problem hiding this comment.
test cases with default width and a minwidth
|
|
||
| const props = Object.assign({}, baseProps, { | ||
| content_style: 'grow', | ||
| columns: columns.map((id => ({ id: id, name: id.toUpperCase(), width: '33%' }))), |
There was a problem hiding this comment.
test cases with percentage based widths
| }; | ||
|
|
||
| const props = Object.assign({}, baseProps, { | ||
| columns: columns.map((id => ({ id: id, name: id.toUpperCase(), width: 20 }))) |
There was a problem hiding this comment.
test cases with width only
|
Copy-paste tests are failing for me locally, here's the output: |
|
@valentijnnieman Attempting to reproduce the issue.. ran it with npm run test.watch or circleci cli? |
| {...attributes} | ||
| />); | ||
| (<div className='dash-input-cell-value-container dash-cell-value-container'> | ||
| {this.renderValue({ className: 'input-cell-value-shadow cell-value-shadow' })} |
There was a problem hiding this comment.
Could you explain this shadow div stuff for me in more detail? Looks interesting, but not sure why it is needed.
| @@ -175,7 +188,10 @@ export default class CellInput extends PureComponent<ICellProps, ICellState> { | |||
|
|
|||
| if (dropdown && document.activeElement !== dropdown) { | |||
| // Limitation. If React >= 16 --> Use React.createRef instead to pass parent ref to child | |||
| (dropdown.wrapper.parentElement as HTMLElement).focus(); | |||
| const tdParent = DOM.getFirstParentOfType(dropdown.wrapper, 'td'); | |||
There was a problem hiding this comment.
Maybe here you could try using refs too instead of manual DOM searching, they're not a React 16 specific feature, here's how they're used in 15 I believe: https://reactjs.org/docs/refs-and-the-dom.html#callback-refs
| @@ -178,7 +178,7 @@ export default class FilterFactory { | |||
| R.addIndex<IVisibleColumn, JSX.Element>(R.map)((column, index) => { | |||
| return (<ColumnFilter | |||
| key={`column-${index}`} | |||
| classes={`filter column-${index}`} | |||
| classes={`dash-filter column-${index}`} | |||
There was a problem hiding this comment.
Just wondering about if the column class should be prefixed with dash- too, since it's a pretty standard class name for most CSS grid frameworks
There was a problem hiding this comment.
I actually do not want column-${index} to be considered standard for API purposes as it exposes internal logic (hidden/visible columns, etc.). I could add a data-dash-column={id} here though.
| @@ -280,13 +280,137 @@ def layout(): | |||
| section_title("Dash Table - Padding"), | |||
| # ... | |||
| section_title("Dash Table - All Column Widths by Percent"), | |||
| # ... | |||
| html.Div( | |||
There was a problem hiding this comment.
No worries, as long as all the examples are there :) (looks to be the case, except for the others that aren't merged yet I guess?)
|
@Marc-Andre-Rivet with |
|
@valentijnnieman npm test is sadly not self-contained at this moment, could you run this instead and let me know if it works. npm run build:js-test && npm test I don't to get into updating the circleci script as part of this but keeping it in mind. |
chriddyp
reviewed
Oct 10, 2018
| "rule": "white-space: normal", | ||
| } | ||
| ], | ||
| ), | ||
| section_title("Dash Table - Content with Ellipses"), | ||
| # ... |
There was a problem hiding this comment.
Are you planning on filling out the rest of these too?
There was a problem hiding this comment.
Honestly I did not and focused on the width ones. Can this be done as a follow up?
| @@ -178,7 +178,8 @@ export default class FilterFactory { | |||
| R.addIndex<IVisibleColumn, JSX.Element>(R.map)((column, index) => { | |||
| return (<ColumnFilter | |||
| key={`column-${index}`} | |||
| classes={`filter column-${index}`} | |||
| classes={`dash-filter column-${index}`} | |||
| columnId={column.id} | |||
There was a problem hiding this comment.
@valentijnnieman New prop with the column id that will be used to create the data-dash-column attribute
|
@valentijnnieman I am unable to reproduce the test issues you have been having when executing build:js-test prior to npm test. This runs fine in CircleCI. Not sure what is happening. |
valentijnnieman
approved these changes
Oct 11, 2018
Marc-Andre-Rivet
added a commit
that referenced
this pull request
Oct 11, 2018
* 3.1 props refactoring (#106) * refactor virtualization, virtualization settings, and derived props * refactor renaming paging / pagination props * refactor virtual, viewport and pagination into adapters * isolate derived props * build update * fix regression * improve typing * fix test regression * simplify pagination adapter / refactor * lint * clean up unused props * - change factory - clean up props / build update * fix lint * bump version * add dash level props for virtual_dataframe * refactor fp / derived props * derived props * refactor viewport and virtual controlled props * fix fp regression * fix fp regression * refactor controlled table / table fp * controlled table purecomponent * fix test (rebrake it!) * fix selection regression for be paging/sorting/filtering * improve re-renders & controlled props * fix test regressions * update inner-selection fixture * remove useless spy * - fix pr comment - fix for IE/Edge * clean up * 3.0 clean offsets (#110) * refactor virtualization, virtualization settings, and derived props * refactor renaming paging / pagination props * refactor virtual, viewport and pagination into adapters * isolate derived props * build update * fix regression * improve typing * fix test regression * simplify pagination adapter / refactor * lint * clean up unused props * - change factory - clean up props / build update * fix lint * bump version * add dash level props for virtual_dataframe * cleaup offsets * triad validation * - define external facing classes and attributes * fix regression, update build * fix test regression (invalid props) * update test name * refactor fp / derived props * derived props * refactor viewport and virtual controlled props * fix fp regression * fix fp regression * refactor controlled table / table fp * controlled table purecomponent * fix test (rebrake it!) * fix selection regression for be paging/sorting/filtering * improve re-renders & controlled props * fix test regressions * update inner-selection fixture * remove useless spy * - control columns into visible columns - cleanup "hidden" conditional processing * update changelog * clean up header factory * apply style on first controlled table render * typo/merge miss * derived visible columns * - visual tests for hidden columns * rename functions * - fix dropdown styling regression * lint * 3.1 props fixes (#112) * props fixes * update changelog * bump version * filter typing * Update derivedViewport_test.ts Add basic viewport test with |df| > page_size * ☝️ 3 new review / documentation / target apps see discussion in #108 * 🙈 forgot file * 📝 incorporate @cldougl suggestions 🙇 * 3.1 refactor tests (#113) * props fixes * update changelog * bump version * filter typing * - delete unused usage files - restructure tests folder * - separate cypress and visual tests into 2 ci jobs * - build before tests! * add browsers to the node image for visual-test * 💯 add percent examples * 📝 title/example clarification * reformat sizing test app for failing tests (#125) * Removed .only from dash_test.ts * Production build instead of dev 🙈 * Fix failing tests * 3.1 refactor cells rendering (#123) * WIP - memoize cell event handlers as derived values - isolate cell event handlers * wip - attempt to isolate cell logic from input logic for individual datum * wip - celan up cell wrapper - isolate input logic a bit more * further down the rabbit hole.. - separating operation cells from content * fix dropdown navigation regression * fix selected row regression * renaming / restructuring * rename/restructure * - clean up zipping - separate wrappers from styles * rework style/ast cache * clean up * clean up * build update * improve rendering perf * optimize wrappers generation * build config * - fix typing regression - fix rendering perf regression * - fix navigation regression * simplify slightly the derived props / ui * fix copy/paste regressions * clean up wrapper props * clean up * fix regression on conditional dropdowns * wip, fp the headers * fp content, wrappers, labels, indices from header factory * fix regressions * fp the table itself * fix typing and behavior for table fp * fix sorting icon regression * fix regression * regression * fix column name regression with only 1 header row * fix header actions regression * fix style application on mount * fix regression edit cell in n-th page * fix editing on non-first page (continued) * fix test * 3.1 issue118 width behavior (#130) * WIP - memoize cell event handlers as derived values - isolate cell event handlers * wip - attempt to isolate cell logic from input logic for individual datum * wip - celan up cell wrapper - isolate input logic a bit more * further down the rabbit hole.. - separating operation cells from content * fix dropdown navigation regression * fix selected row regression * renaming / restructuring * rename/restructure * - clean up zipping - separate wrappers from styles * rework style/ast cache * clean up * clean up * build update * improve rendering perf * optimize wrappers generation * build config * - fix typing regression - fix rendering perf regression * - fix navigation regression * simplify slightly the derived props / ui * fix copy/paste regressions * clean up wrapper props * clean up * fix regression on conditional dropdowns * wip, fp the headers * fp content, wrappers, labels, indices from header factory * fix regressions * fp the table itself * fix typing and behavior for table fp * fix sorting icon regression * fix regression * regression * fix column name regression with only 1 header row * fix header actions regression * add width percentage support + content_style * fix style application on mount * fix visual regression with empty df * only apply row style when necessary * fix tab test (no offset) * clean up header styling * use dash-* classes * support default column width (override input behavior) * fix regression edit cell in n-th page * fix editing on non-first page (continued) * fix test * fit to content behavior * fix regressions * fix lint * add column width visual tests * fix dropdown minimum size when using default width * sizing examples * black * fix navigation test regression * fix regressions in visual tests * default column width - fix dropdown width eval * default width columns - fix width when first content row is search filter * percy - add delay before screenshot (attempt to fix FF visual tests) * debugging selenium * fix black * debug selenium * debug selenium * fix black * debug selenium * debug selenium * debug selenium * undo all selenium modifications * default column width - filter inputs behave like cell inputs (do not affect width) * - fixed rows+columns height evaluated correctly * remove dead code * remove .only from tests * add data-dash-column to filter cells * styling examples (#117) * 🌈 styling examples examples that represent of the level of customization that we need in dash-table. The examples are implemented with HTML tables to demonstrate the intended behaviour. I’d like to see each of these examples implemented with the `dash_table` syntax. We’ll use these examples as our `dash-table` documentation * ❌ removing black and pylint this keeps tripping us up and I don’t think it’s worth the pain right now. * Backend examples (#119) * 🏭 backend computed data usage examples * 📊 tying it together w a graph * ❓ not sure what `displayed_pages` does? * Dropdown usage examples (#120) * ⬇️ dropdown usage examples * add conditional dropdown example
Marc-Andre-Rivet
added a commit
that referenced
this pull request
Oct 12, 2018
* 3.1 props refactoring (#106) * refactor virtualization, virtualization settings, and derived props * refactor renaming paging / pagination props * refactor virtual, viewport and pagination into adapters * isolate derived props * build update * fix regression * improve typing * fix test regression * simplify pagination adapter / refactor * lint * clean up unused props * - change factory - clean up props / build update * fix lint * bump version * add dash level props for virtual_dataframe * refactor fp / derived props * derived props * refactor viewport and virtual controlled props * fix fp regression * fix fp regression * refactor controlled table / table fp * controlled table purecomponent * fix test (rebrake it!) * fix selection regression for be paging/sorting/filtering * improve re-renders & controlled props * fix test regressions * update inner-selection fixture * remove useless spy * - fix pr comment - fix for IE/Edge * clean up * 3.0 clean offsets (#110) * refactor virtualization, virtualization settings, and derived props * refactor renaming paging / pagination props * refactor virtual, viewport and pagination into adapters * isolate derived props * build update * fix regression * improve typing * fix test regression * simplify pagination adapter / refactor * lint * clean up unused props * - change factory - clean up props / build update * fix lint * bump version * add dash level props for virtual_dataframe * cleaup offsets * triad validation * - define external facing classes and attributes * fix regression, update build * fix test regression (invalid props) * update test name * refactor fp / derived props * derived props * refactor viewport and virtual controlled props * fix fp regression * fix fp regression * refactor controlled table / table fp * controlled table purecomponent * fix test (rebrake it!) * fix selection regression for be paging/sorting/filtering * improve re-renders & controlled props * fix test regressions * update inner-selection fixture * remove useless spy * - control columns into visible columns - cleanup "hidden" conditional processing * update changelog * clean up header factory * apply style on first controlled table render * typo/merge miss * derived visible columns * - visual tests for hidden columns * rename functions * - fix dropdown styling regression * lint * 3.1 props fixes (#112) * props fixes * update changelog * bump version * filter typing * Update derivedViewport_test.ts Add basic viewport test with |df| > page_size * ☝️ 3 new review / documentation / target apps see discussion in #108 * 🙈 forgot file * 📝 incorporate @cldougl suggestions 🙇 * 3.1 refactor tests (#113) * props fixes * update changelog * bump version * filter typing * - delete unused usage files - restructure tests folder * - separate cypress and visual tests into 2 ci jobs * - build before tests! * add browsers to the node image for visual-test * 💯 add percent examples * 📝 title/example clarification * reformat sizing test app for failing tests (#125) * Removed .only from dash_test.ts * Production build instead of dev 🙈 * Fix failing tests * 3.1 refactor cells rendering (#123) * WIP - memoize cell event handlers as derived values - isolate cell event handlers * wip - attempt to isolate cell logic from input logic for individual datum * wip - celan up cell wrapper - isolate input logic a bit more * further down the rabbit hole.. - separating operation cells from content * fix dropdown navigation regression * fix selected row regression * renaming / restructuring * rename/restructure * - clean up zipping - separate wrappers from styles * rework style/ast cache * clean up * clean up * build update * improve rendering perf * optimize wrappers generation * build config * - fix typing regression - fix rendering perf regression * - fix navigation regression * simplify slightly the derived props / ui * fix copy/paste regressions * clean up wrapper props * clean up * fix regression on conditional dropdowns * wip, fp the headers * fp content, wrappers, labels, indices from header factory * fix regressions * fp the table itself * fix typing and behavior for table fp * fix sorting icon regression * fix regression * regression * fix column name regression with only 1 header row * fix header actions regression * fix style application on mount * fix regression edit cell in n-th page * fix editing on non-first page (continued) * fix test * 3.1 issue118 width behavior (#130) * WIP - memoize cell event handlers as derived values - isolate cell event handlers * wip - attempt to isolate cell logic from input logic for individual datum * wip - celan up cell wrapper - isolate input logic a bit more * further down the rabbit hole.. - separating operation cells from content * fix dropdown navigation regression * fix selected row regression * renaming / restructuring * rename/restructure * - clean up zipping - separate wrappers from styles * rework style/ast cache * clean up * clean up * build update * improve rendering perf * optimize wrappers generation * build config * - fix typing regression - fix rendering perf regression * - fix navigation regression * simplify slightly the derived props / ui * fix copy/paste regressions * clean up wrapper props * clean up * fix regression on conditional dropdowns * wip, fp the headers * fp content, wrappers, labels, indices from header factory * fix regressions * fp the table itself * fix typing and behavior for table fp * fix sorting icon regression * fix regression * regression * fix column name regression with only 1 header row * fix header actions regression * add width percentage support + content_style * fix style application on mount * fix visual regression with empty df * only apply row style when necessary * fix tab test (no offset) * clean up header styling * use dash-* classes * support default column width (override input behavior) * fix regression edit cell in n-th page * fix editing on non-first page (continued) * fix test * fit to content behavior * fix regressions * fix lint * add column width visual tests * fix dropdown minimum size when using default width * sizing examples * black * fix navigation test regression * fix regressions in visual tests * default column width - fix dropdown width eval * default width columns - fix width when first content row is search filter * percy - add delay before screenshot (attempt to fix FF visual tests) * debugging selenium * fix black * debug selenium * debug selenium * fix black * debug selenium * debug selenium * debug selenium * undo all selenium modifications * default column width - filter inputs behave like cell inputs (do not affect width) * - fixed rows+columns height evaluated correctly * remove dead code * remove .only from tests * add data-dash-column to filter cells * styling examples (#117) * 🌈 styling examples examples that represent of the level of customization that we need in dash-table. The examples are implemented with HTML tables to demonstrate the intended behaviour. I’d like to see each of these examples implemented with the `dash_table` syntax. We’ll use these examples as our `dash-table` documentation * ❌ removing black and pylint this keeps tripping us up and I don’t think it’s worth the pain right now. * Backend examples (#119) * 🏭 backend computed data usage examples * 📊 tying it together w a graph * ❓ not sure what `displayed_pages` does? * Dropdown usage examples (#120) * ⬇️ dropdown usage examples * add conditional dropdown example * - additional tests for editable/readonly - fixes for editable/readonly * bump version * remove useless test column * add editable prop to fixtures * update tests to take into account readonly 'rows' column * - refactor column isEditable calculation
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
3.1 equivalent to the work done in 3.0 for % width support -- extended functionality to include support for default size columns.
Now.. the HTML table is a special beast and what's set and actual behavior are not always consistent with one another especially when it comes to % based sizes.. and sometimes to min/max/width props too.. if you feel the finer points should be re-discussed in this PR please do not hesitate to say so.
New stuff here: