-
-
Notifications
You must be signed in to change notification settings - Fork 74
fix keyboard navigation after copy/paste #90
Conversation
this.onCopy(e as any); | ||
e.preventDefault(); | ||
/*#endif*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default handling of keys ends up changing the focus state, which affects how future keys are handled. 'Enter' is meant to set the focus "deeper" than the table, on the input itself, but not Ctrl+C / Ctrl+V. Exiting explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what these /*#endif*/
comments are doing? templating in code for different builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The test build has a special build-time variable defined
dash-table/webpack.test.config.js
Line 3 in 5fd75e3
definitions: ['TEST_COPY_PASTE'] |
The loader reads through the files and keeps/removes the blocks based on whether the variable is defined or not. This is useful for defining build specific behavior or configurations that need to be more flexible than overriding an alias/file and/or not have to carry the conditional logic for tests/dev/etc. into production code. Created this loader a while ago when I encountered test cases I couldn't run on Selenium because it wasn't possible to open a file selector and choose a file..
https://www.npmjs.com/package/webpack-preprocessor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically, in this case, Cypress does not have good support of copy & paste events, so instead of letting the browser handle it, triggering the copy/paste processing manually when the key combinations are hit. This creates a gap between the test impl and the real impl but the alternative is to manual test features.
DOM.focused.type(Key.ArrowDown); | ||
DashTable.getCell(4, 3).should('have.class', 'focused'); | ||
DashTable.getCell(3, 3).should('not.have.class', 'focused'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test makes sure that we can navigate out of the cell once Ctrl+C has been pressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃 works as expected now- thanks for the additional test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 💃
Confirmed to fix #49.
this.onCopy(e as any); | ||
e.preventDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more this pops up the more I think we should keep track of custom vs default event handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this one was already there, hidden away in onCopy, just made both consistent :)
No description provided.