Issue 324 - Fix header misalignment regression with "always visible" scrollbar#325
Conversation
| "webpack-dev-server": "^3.1.10", | ||
| "webpack": "^4.28.3", | ||
| "webpack-cli": "^3.2.1", | ||
| "webpack-dev-server": "^3.1.14", |
There was a problem hiding this comment.
This is noise from renovate/all -- we don't commit the build result when updating deps
| if (!virtualization || !uiCell || !uiViewport) { | ||
| return [ | ||
| [{}, {}], | ||
| base, |
There was a problem hiding this comment.
If there is no virtualization or if the virtualization is not ready -- return the scrolling offset only
|
|
||
| return [ | ||
| [{}, { fragment: { marginRight } }], | ||
| base, |
There was a problem hiding this comment.
Otherwise, return offset + virtualization-specific styling
|
is it much work to put in a regression test? |
|
@bpostlethwaite The best I can do is add a unit test to make sure the logic is sound, which I'll add now. There's no easy way to test this in e2e/integration atm. |
valentijnnieman
left a comment
valentijnnieman
left a comment
There was a problem hiding this comment.
Small but important change - looks good to me 💃
| scrollbarWidth | ||
| ); | ||
|
|
||
| validate(style, scrollbarWidth); |
There was a problem hiding this comment.
Basic unit tests to make sure (not-)virtualized table (not-)ready always returns the correct styling for the header alignment / offset.
or maybe part of the review app for manual testing / qa? |
| "supportFile": "./tests/cypress/support/index.js", | ||
| "videoFolder": "./tests/cypress/videos" | ||
| "videoFolder": "./tests/cypress/videos", | ||
| "numTestsKeptInMemory": 0 |
There was a problem hiding this comment.
Some annoying tidbits related to Cypress -- it seems the additional tests tipped it over into Heap Overflow land.
| "private::runtests-v1:unit": "cypress run --browser chrome --spec 'tests/cypress/tests/unit/**/*'", | ||
| "private::runtests-v1:standalone": "cypress run --browser chrome --spec 'tests/cypress/tests/standalone/**/*'", | ||
| "private::runtests-v1:server": "cypress run --browser chrome --spec 'tests/cypress/tests/server/**/*'", | ||
| "private::runtests-v1": "run-s private::runtests-v1:unit private::runtests-v1:standalone private::runtests-v1:server", |
There was a problem hiding this comment.
Some annoying tidbits related to Cypress -- it seems the additional tests tipped it over into Heap Overflow land. Break up to test into sections for now. Should probably explore parallelisation in the future.
Yes! As we plan on linking all core dash-* repos versions together in the future, adding documentation for this in dash-core (https://github.com/plotly/dash-core/blob/master/Manual%20Testing.md) and adding an entry for PR review checklist to that effect (https://github.com/plotly/dash-core/blob/master/PR%20Checklist.md#test-review) |
4 participants
Fix #324.
Virtualization #253 introduced a regression for the table's fragments style generation. Specifically, the style is not generated when the table is not virtualized!