New: Added support for keyboard interaction #76

parmar-abhinav · 2023-08-09T13:36:48Z

This PR introduces and new feature that allows user to Drag/Zoom graph in Canvas using keyboard. Allowing users to control the scale of zoom using zoomInFactor and zoomOutFactor also draggingFactor allows to control dragging speed. This feature enhances the usability of the library by giving users more control over interactivity.

tonilastre

Nice feature!

tonilastre · 2023-08-17T10:58:25Z

src/views/orb-view.ts

+  _dragByArrowKey = (transform: ZoomTransform) => {
+    select(this._canvas)
+      .transition()
+      .duration(this._settings.zoomFitTransitionMs)
+      .ease(easeLinear)
+      .call(this._d3Zoom.transform, transform)
+      .call(() => {
+        this._renderer.render(this._graph);
+      });
+  };


Suggested change

_dragByArrowKey = (transform: ZoomTransform) => {

select(this._canvas)

.transition()

.duration(this._settings.zoomFitTransitionMs)

.ease(easeLinear)

.call(this._d3Zoom.transform, transform)

.call(() => {

this._renderer.render(this._graph);

});

};

_applyTransformation = (transform: ZoomTransform, callback?: () => void) => {

select(this._canvas)

.transition()

.duration(this._settings.zoomFitTransitionMs)

.ease(easeLinear)

.call(this._d3Zoom.transform, transform)

.call(() => {

this._renderer.render(this._graph);

callback?.();

});

};

This doesn't have anything to do with arrows. It is applying the transformation to the canvas. Also, this code is pretty much the same as the one used in recenter(), so I would advise changing the one in recenter to use this:

recenter(onRendered?: () => void) { const fitZoomTransform = this._renderer.getFitZoomTransform(this._graph); this._applyTransformation(fitZoomTransform, () => { onRendered?.(); }); }

Thank you for your feedback. I've made the necessary changes based on your suggestions. Pls check.

tonilastre · 2023-08-17T10:59:03Z

docs/view-default.md

@@ -157,6 +163,12 @@ const defaultSettings = {
  interaction: {
    isDragEnabled: true;
    isZoomEnabled:  true;
+    keyboard: {
+      isKeyboardEnabled: true,


As far as I can see, default is false.

Also, you already have a group name keyboard, so this field can just be isEnabled.

Yes by default it is false. Renamed isKeyboardEnabled to isEnabled and also update default value

tonilastre · 2023-08-17T11:00:51Z

src/renderer/canvas/canvas-renderer.ts

+  getDragLeftTransform(draggingFactor: number): ZoomTransform {
+    const currentTransform = this.transform;
+    const newX = currentTransform.x + draggingFactor;
+    return zoomIdentity
+      .translate(newX, currentTransform.y)
+      .scale(currentTransform.k);
+  }
+
+  getDragRightTransform(draggingFactor: number): ZoomTransform {
+    const currentTransform = this.transform;
+    const newX = currentTransform.x - draggingFactor;
+    return zoomIdentity
+      .translate(newX, currentTransform.y)
+      .scale(currentTransform.k);
+  }
+
+  getDragUpTransform(draggingFactor: number): ZoomTransform {
+    const currentTransform = this.transform;
+    const newY = currentTransform.y + draggingFactor;
+    return zoomIdentity
+      .translate(currentTransform.x, newY)
+      .scale(currentTransform.k);
+  }
+
+  getDragDownTransform(draggingFactor: number): ZoomTransform {
+    const currentTransform = this.transform;
+    const newY = currentTransform.y - draggingFactor;
+    return zoomIdentity
+      .translate(currentTransform.x, newY)
+      .scale(currentTransform.k);
+  }


All these are pretty much doing the same thing, so I would propose having one function that handles all 4 cases with enum, e.g. PanDirectionType.{UP, DOWN, LEFT, RIGHT}.

Btw this event should be pan, drag is used for dragging nodes/edges. Pan is when you move the canvas up/down/left/right.
With the enum, you can have just one function getPanTransform(panDirectionType, factor);

Created single getPanTransform function, Pls check.

tonilastre · 2023-08-17T11:01:16Z

src/views/orb-view.ts

@@ -364,6 +390,80 @@ export class OrbView<N extends INodeBase, E extends IEdgeBase> implements IOrbVi
    }, 1);
  };

+  _handleKeyDown = (event: KeyboardEvent) => {


This is missing on the orb-map-view.

"orb-map-view" is already responding to key presses, so I decided not to make any changes.

Awesome! Yep, that makes sense.

…n directions

tonilastre

It is better and more performant code now! Btw I have some comments mostly about moving stuff to other files, missing property, and handling key events globally.

tonilastre · 2023-08-22T22:45:37Z

src/renderer/canvas/canvas-renderer.ts

@@ -25,6 +25,13 @@ const DEBUG_GREEN = '#3CFF33';
 const DEBUG_BLUE = '#3383FF';
 const DEBUG_PINK = '#F333FF';

+export enum PanDirectionType {


This should go to src/renderer/shared.ts because all renderer classes will share this.

tonilastre · 2023-08-22T22:56:42Z

src/views/orb-view.ts

+    switch (event.key) {
+      case '-': {
+        const zoomOutFactor = this._settings.interaction.keyboard.zoomOutFactor;
+        this._zoomOut(zoomOutFactor);
+        break;
+      }
+      case '+': {
+        const zoomInFactor = this._settings.interaction.keyboard.zoomInFactor;
+        this._zoomIn(zoomInFactor);
+        break;
+      }
+      case 'ArrowLeft': {
+        const panFactor = this._settings.interaction.keyboard.panFactor;
+        const dragLeftTransform = this._renderer.getPanTransform(PanDirectionType.LEFT, panFactor);
+        this._applyTransformation(dragLeftTransform);
+        break;
+      }
+      case 'ArrowRight': {
+        const panFactor = this._settings.interaction.keyboard.panFactor;
+        const dragRightTransform = this._renderer.getPanTransform(PanDirectionType.RIGHT, panFactor);
+        this._applyTransformation(dragRightTransform);
+        break;
+      }
+      case 'ArrowUp': {
+        const panFactor = this._settings.interaction.keyboard.panFactor;
+        const dragUpTransform = this._renderer.getPanTransform(PanDirectionType.UP, panFactor);
+        this._applyTransformation(dragUpTransform);
+        break;
+      }
+      case 'ArrowDown': {
+        const panFactor = this._settings.interaction.keyboard.panFactor;
+        const dragDownTransform = this._renderer.getPanTransform(PanDirectionType.DOWN, panFactor);
+        this._applyTransformation(dragDownTransform);
+        break;
+      }
+      default:
+        break;
+    }


Nitpicking, but because there are 6 long statements to get the zoomInFactor, zoomOutFactor and panFactor, it makes sense to have variable destruct before the switch case, e.g.

const { zoomOutFactor, zoomInFactor, panFactor } = this._settings.interaction.keyboard; switch (event.key) { case '-': { this._zoomOut(zoomOutFactor); break; } ... }

With this, you can literally remove lines where you get the factors in each switch-case.

I've made the recommended changes to improve the code readability and reduce redundancy.

tonilastre · 2023-08-22T23:07:25Z

src/views/orb-view.ts

+  _applyTransformation = (transform: ZoomTransform, callback?: () => void) => {
+    select(this._canvas)
+      .transition()
+      .duration(this._settings.zoomFitTransitionMs)


Sorry, I totally missed this one in the last PR check. This this._settings.zoomFitTransitionMs is actually from the recenter function that does fit to zoom.

In order to make this applyTransformation generic enough, I propose having a second argument options (that will currently have transitionMs (+ callback) only. Something like this:

interface IApplyTransitionOptions { transitionMs: number; callback: () => void; } _applyTransformation = (transform: ZoomTransform, options?: Partial<IApplyTransitionOptions>) => { const transitionMs = options?.transitionMs ?? DEFAULT_TRANSITION_MS; ...

Then, the recenter can send its this._settings.zoomFitTransitionMs and callback. The zoom, and pan can send their transitionMs. This transitionMs can be even added to the settings with a default value that is lower than 200, maybe 100 or 50. 200 seems a bit slow for expected keyboard movement.

I've implemented your suggested approach to enhance the generic nature of the applyTransformation function.

Thank you for your valuable input.

tonilastre · 2023-08-22T23:12:00Z

src/views/orb-view.ts

@@ -150,6 +168,8 @@ export class OrbView<N extends INodeBase, E extends IEdgeBase> implements IOrbVi
      .on('contextmenu', this.mouseRightClicked)
      .on('dblclick.zoom', this.mouseDoubleClicked);

+    document.addEventListener('keydown', this._handleKeyDown);


Ok, this is a bit problematic, to listen for the key down events on the whole document. If there is a form next to the Orb view, orb will get each event and even react to +, -, array keys while the user writes keys in the input.

So, this should be fixed either by:

Listening only for keys on the container - I don't think this is even possible

Having a way to listen for click events in the container to make it "focused" so we can know if we want to react to document.keydown event (for example, check the orb-view-map and how it works - key events are working as expected there: if you click on a map, it becomes "focused" in the background, and keys are working. If you click on the header, keys are not working for the graph view out of the orb view, which is the expected behavior).

If you have any other suggestions on how to approach this problem, feel free to share. Happy to discuss this.

@parmar-abhinav do you have any info on this?

Tried few approach i.e.

Instead of attaching the keydown event listener to the entire document. I tried attaching it to the specific container where we want to handle the key events. But not able to listen to keydown event using this approach.

this._container.addEventListener('keydown', (event) => {})

Also tried listening to focusin and focusout event on the container. But it is also not working.

private readonly isContainerFocused = false; this._container.addEventListener('focusin', () => { this.isContainerFocused = true; }); this._container.addEventListener('focusout', () => { this.isContainerFocused = false; }); document.addEventListener('keydown', event => { if (this.isContainerFocused) { // Handle keydown events only when the container is focused this._handleKeyDown(event); } });

Hmm, thanks for sharing. I will check how the leaflet did it (it is used for orb-view-map), maybe we can replicate that behavior. I will share my findings here.

tonilastre · 2023-08-22T23:12:15Z

src/views/orb-view.ts

@@ -364,6 +390,80 @@ export class OrbView<N extends INodeBase, E extends IEdgeBase> implements IOrbVi
    }, 1);
  };

+  _handleKeyDown = (event: KeyboardEvent) => {


Awesome! Yep, that makes sense.

tonilastre

@parmar-abhinav everything looks great, there are just two comments left: one for default and another one for document-level listening (which might be problematic when integrating orb). Please check those two and let me know.

tonilastre · 2023-08-31T19:44:35Z

src/views/orb-view.ts

+const DEFAULT_ZOOM_IN_FACTOR = 1.2;
+const DEFAULT_ZOOM_OUT_FACTOR = 0.8;
+const DEFAULT_PAN_FACTOR = 25;
+const DEFAULT_TRANSITION_MS = 200;


Maybe the default can be 0 - no transition.

Done, Please check

tonilastre · 2023-08-31T19:44:49Z

src/views/orb-view.ts

@@ -150,6 +168,8 @@ export class OrbView<N extends INodeBase, E extends IEdgeBase> implements IOrbVi
      .on('contextmenu', this.mouseRightClicked)
      .on('dblclick.zoom', this.mouseDoubleClicked);

+    document.addEventListener('keydown', this._handleKeyDown);


@parmar-abhinav do you have any info on this?

New: Added support for keyboard interaction

3b6d84f

parmar-abhinav requested review from cizl and tonilastre as code owners August 9, 2023 13:36

tonilastre requested changes Aug 17, 2023

View reviewed changes

tonilastre added the enhancement label Aug 17, 2023

parmar-abhinav added 2 commits August 19, 2023 12:36

Chore: Apply linting to enhance code quality

291573b

Refactor: Streamline canvas transformations and introduce enum for pa…

8d388e3

…n directions

tonilastre requested changes Aug 22, 2023

View reviewed changes

parmar-abhinav added 2 commits August 27, 2023 18:06

Refactor: Optimize shared renderer logic and improve variable usage

5948934

Enhancement: Introduce transition options for generic transformation

837d477

tonilastre requested changes Aug 31, 2023

View reviewed changes

Update: Modified default value of transition

8bde61a

hal-eisen-MG added feature and removed enhancement labels Nov 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Added support for keyboard interaction #76

New: Added support for keyboard interaction #76

parmar-abhinav commented Aug 9, 2023

tonilastre left a comment

tonilastre Aug 17, 2023

parmar-abhinav Aug 19, 2023

tonilastre Aug 17, 2023

tonilastre Aug 17, 2023

parmar-abhinav Aug 19, 2023

tonilastre Aug 17, 2023

parmar-abhinav Aug 19, 2023

tonilastre Aug 17, 2023

parmar-abhinav Aug 19, 2023

tonilastre Aug 22, 2023

tonilastre left a comment

tonilastre Aug 22, 2023

tonilastre Aug 22, 2023

tonilastre Aug 22, 2023

parmar-abhinav Aug 27, 2023

tonilastre Aug 22, 2023

parmar-abhinav Aug 27, 2023

tonilastre Aug 22, 2023

tonilastre Aug 31, 2023

parmar-abhinav Sep 1, 2023

tonilastre Sep 4, 2023

tonilastre Aug 22, 2023

tonilastre left a comment

tonilastre Aug 31, 2023

parmar-abhinav Sep 1, 2023

tonilastre Aug 31, 2023

New: Added support for keyboard interaction #76

Are you sure you want to change the base?

New: Added support for keyboard interaction #76

Conversation

parmar-abhinav commented Aug 9, 2023

tonilastre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonilastre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonilastre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment