Add test for selection of cells outside of the viewport. #535
Conversation
shammamah-zz
force-pushed
the
viewport-error-test
branch
from
d149ddb
to
8648d77
Compare
shammamah-zz
force-pushed
the
viewport-error-test
branch
from
8648d77
to
78d409a
Compare
shammamah-zz
force-pushed
the
viewport-error-test
branch
from
78d409a
to
f7d52f0
Compare
shammamah-zz
force-pushed
the
viewport-error-test
branch
from
f7d52f0
to
59dfefe
Compare
shammamah-zz
force-pushed
the
viewport-error-test
branch
from
59dfefe
to
18f7593
Compare
Change from using the virtualized row (i.e., the row index in the viewport).
shammamah-zz
force-pushed
the
viewport-error-test
branch
from
741eee9
to
5a300c0
Compare
| @@ -28,7 +28,7 @@ class Wrappers { | |||
| (column, columnIndex) => this.getWrapper( | |||
| false, | |||
| false, | |||
| rowIndex, | |||
| rowIndex + _offset.rows, | |||
There was a problem hiding this comment.
You can rename _offset to offset. The underscore was there to prevent tslint from flagging it as an unused parameter since it was required for caching purposes but not when calculating.
| for (let i = 0; i < 25; i++) { | ||
| DOM.focused.type(Key.ArrowDown); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Nothing specific is being validated here. If the down arrow causes an error and/or does not update the selection the test will not fail.
There was a problem hiding this comment.
I tested this with the version of the table that had this error, and the test did end up failing, since there is a console error (which I thought would be implicitly included in the test failure conditions). Is there another way to catch console errors?
There was a problem hiding this comment.
Ah! You're right. Unhandled errors cause the tests to fail by default - did not realize that was the case.
https://docs.cypress.io/api/events/catalog-of-events.html#To-turn-off-all-uncaught-exception-handling
There was a problem hiding this comment.
Would still like the test do more validation. The implementation could change and the error be swallowed instead of being unhandled. The test would not fail in that case.
| @@ -100,6 +107,14 @@ Object.values(BasicModes).forEach(mode => { | |||
| DashTable.getCell(4, 2).should('have.class', 'focused'); | |||
| DashTable.getCell(3, 1).should('not.have.class', 'focused'); | |||
| }); | |||
|
|
|||
| it('can select a cell outside of the viewport', () => { | |||
There was a problem hiding this comment.
I think the test name could be improved. It's not about selecting a cell outside of viewport but rather about selecting a cell and scrolling it out of the viewport.
Like for the test above, would be interested in checking if there are console errors triggered by these steps. Maybe something like: cypress-io/cypress#300 (comment)
There was a problem hiding this comment.
@shammamah From your comment in https://github.com/plotly/dash-table/pull/535/files#r315342988, the console error part is moot.
data-dash-column in this case is a string that refers to the ID of the column, and not the index.
shammamah-zz
force-pushed
the
viewport-error-test
branch
from
399409e
to
542be91
Compare
Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>
Closes #520.
About
data-dash-rowto point to the actual row in the data as opposed to the row number in the viewport.