Conversation
- use preset-env
| Merge<PropsWithDefaults, { | ||
| fixed_columns: number; | ||
| fixed_rows: number; | ||
| loading_state: boolean; |
There was a problem hiding this comment.
Synthetize the loading_state object to a boolean representing whether data is being worked on. Prevents a bunch of useless re-renders.
| expectCellSelection([1, 2, 3], [3001, 3002, 3003], [0, 1], [0, 1], [2, 1], [2, 1]); | ||
|
|
||
| // shrink the selection | ||
| DOM.focused.type(Key.Shift, { release: false }); |
There was a problem hiding this comment.
Cypress 3.6.x API changed in subtle ways, doing two consecutive non-release shift key presses is actually two presses, needs to be removed for the selection to behave as expected.
| // *********************************************************** | ||
|
|
||
| // Import commands.js using ES2015 syntax: | ||
| import '@babel/polyfill'; |
There was a problem hiding this comment.
The server build gets polyfills from the renderer, the standalone build adds it to the entrypoint. Unit tests get it from cypress, here.
| if (!currentApplyFocus) { | ||
| nextState.applyFocus = true; | ||
| } | ||
| } else if (nextProps.loading_state !== this.props.loading_state) { |
There was a problem hiding this comment.
you're going to have to explain this one to me... why does entering or exiting loading trigger this check?
There was a problem hiding this comment.
If a callback modifying data is triggered and the focus is on a table cell, the table needs to be re-rendered once the callback returns to make the cell writeable again.
…s on callback completion
| .find('.dash-input-cell-value-container > input').should('have.length', 1); | ||
| .within(() => cy.get('.dash-cell-value') | ||
| .should('have.value', 'Hello')); | ||
| }); |
There was a problem hiding this comment.
Made the test a bit tighter by skipping Enter / Click, gives the table less chance to render multiple times and hide issues for renders with a smaller footprint (less props changed)
There was a problem hiding this comment.
I'm not excited about the cy.wait(5000) statements that are proliferating here... I won't block on this, given all the cypress headaches already in this PR, but it would be great if at some point we could figure out a condition to wait for rather than a time.
There was a problem hiding this comment.
Reverting. I ran this in 3.4.1 but in 3.6.1 I'm encountering weird timing issues.
There was a problem hiding this comment.
About the cy.wait(5000) the callback waits for 5 seconds and the maximum default timeout for a selector to succeed in Cypress is 4000ms. It is a bit yucky. Maybe we could rewrite the tests with a timeout override or make the callback shorter -- as opposed to the Selenium tests, these tests don't have access to the Python callback. One definite disadvantage to running e2e tests in Cypress.
alexcjohnson
left a comment
There was a problem hiding this comment.
Looks great, thanks for the extra tests of loading/editing. Once these tests all pass 😅 this looks ready to me! 💃
Sibling of plotly/dash#1006
Also fixes #641