Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(keyboard): bind-method typing still requires EventTarget #948

Merged
merged 2 commits into from
Nov 15, 2024
Merged

fix(keyboard): bind-method typing still requires EventTarget #948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a8434a
fix(keyboard): correct `Keyboard#bind` and config signature
torge-hmn Nov 11, 2024
8ae381a
docs(keyboard): detail deprecation reason
nikku Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/features/keyboard/Keyboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ var compatMessage = 'Keyboard binding is now implicit; explicit binding to an el
* `keyboard.bind=true|false` configuration option.
*
* @param {Object} config
* @param {EventTarget} [config.bindTo]
* @param {boolean} [config.bind]
* @param {EventBus} eventBus
*/
export default function Keyboard(config, eventBus) {
Expand Down Expand Up @@ -125,10 +125,17 @@ Keyboard.prototype._isEventIgnored = function(event) {
/**
* Bind keyboard events to the given DOM node.
*
* @overlord
* @deprecated No longer in use since version 15.0.0.
*
* @param {EventTarget} node
*/
/**
* Bind keyboard events to the canvas node.
*/
Keyboard.prototype.bind = function(node) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should rather fix the method signature, not require any argument, and check whether an argument is provided via arguments[0]. This change looks odd.

Suggested change
Keyboard.prototype.bind = function(node) {
Keyboard.prototype.bind = function() {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed via 36316be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better solution, thank you! 👍 (I wasn't sure if I could change the signature tbh)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to ensure that any signature change is tested; in this case I marked the old behavior as deprecated; it will throw at run-time. In the future we want to remove it all together.


// legacy <node> argument provided
if (node) {
console.error('unsupported argument <node>', new Error(compatMessage));
}
Expand Down
12 changes: 12 additions & 0 deletions lib/features/keyboard/Keyboard.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import Diagram from '../../Diagram';
import Keyboard from './Keyboard';

const diagram = new Diagram();

let keyboard = diagram.get<Keyboard>('keyboard');

keyboard.bind();

keyboard.unbind();

keyboard.bind(document);