Skip to content

Commit

Permalink
[IMP] components: use a hook to register/clean up event listeners
Browse files Browse the repository at this point in the history
Currently, we register event listener on `Ref.el` upong mount and
unmount steps of the component lifecycle.

However, depending on the component structure, the element of the
`Ref` could change from a rendering to another*, preventing the proper
cleaing of the listener.

This commit introduces a new hook to handle such cases more correctly.

* an example can be found in component `SelectionInput`.

closes #2469

Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
  • Loading branch information
rrahir committed May 19, 2023
1 parent 3a3aab7 commit 40b3703
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 26 deletions.
34 changes: 8 additions & 26 deletions src/components/grid_overlay/grid_overlay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
SpreadsheetChildEnv,
} from "../../types";
import { FiguresContainer } from "../figures/figure_container/figure_container";
import { useRefListener } from "../helpers/listener_hook";
import { useInterval } from "../helpers/time_hooks";

function useCellHovered(
Expand Down Expand Up @@ -59,21 +60,10 @@ function useCellHovered(
}
}

onMounted(() => {
const grid = gridRef.el!;
grid.addEventListener("mousemove", updateMousePosition);
grid.addEventListener("mouseleave", pause);
grid.addEventListener("mouseenter", resume);
grid.addEventListener("mousedown", recompute);
});

onWillUnmount(() => {
const grid = gridRef.el!;
grid.removeEventListener("mousemove", updateMousePosition);
grid.removeEventListener("mouseleave", pause);
grid.removeEventListener("mouseenter", resume);
grid.removeEventListener("mousedown", recompute);
});
useRefListener(gridRef, "mousemove", updateMousePosition);
useRefListener(gridRef, "mouseleave", pause);
useRefListener(gridRef, "mouseenter", resume);
useRefListener(gridRef, "mousedown", recompute);

function setPosition(col?: number, row?: number) {
if (col !== hoveredPosition.col || row !== hoveredPosition.row) {
Expand Down Expand Up @@ -120,17 +110,9 @@ function useTouchMove(
y = currentY;
}

onMounted(() => {
gridRef.el!.addEventListener("touchstart", onTouchStart);
gridRef.el!.addEventListener("touchend", onTouchEnd);
gridRef.el!.addEventListener("touchmove", onTouchMove);
});

onWillUnmount(() => {
gridRef.el!.removeEventListener("touchstart", onTouchStart);
gridRef.el!.removeEventListener("touchend", onTouchEnd);
gridRef.el!.removeEventListener("touchmove", onTouchMove);
});
useRefListener(gridRef, "touchstart", onTouchStart);
useRefListener(gridRef, "touchend", onTouchEnd);
useRefListener(gridRef, "touchmove", onTouchMove);
}

interface Props {
Expand Down
23 changes: 23 additions & 0 deletions src/components/helpers/listener_hook.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { useEffect } from "@odoo/owl";
import { Ref } from "../../types";

/**
* Manages an event listener on a ref. Useful for hooks that want to manage
* event listeners, especially more than one. Prefer using t-on directly in
* components. If your hook only needs a single event listener, consider simply
* returning it from the hook and letting the user attach it with t-on.
*
* Adapted from Odoo Community - See https://github.com/odoo/odoo/blob/saas-16.2/addons/web/static/src/core/utils/hooks.js
*/
export function useRefListener(
ref: Ref<HTMLElement>,
...listener: Parameters<typeof addEventListener>
) {
useEffect(
(el: HTMLElement | null) => {
el?.addEventListener(...listener);
return () => el?.removeEventListener(...listener);
},
() => [ref.el]
);
}

0 comments on commit 40b3703

Please sign in to comment.