nteract / semiotic

A data visualization framework combining React & D3
https://semioticv1.nteract.io/
Other
2.43k stars 132 forks source link

Replace element-resize-event #592

Closed alexeyraspopov closed 2 years ago

alexeyraspopov commented 2 years ago

As I got stuck with https://github.com/nteract/semiotic/pull/588#issuecomment-1000453858 and then figured that I can easily fixed the issue by doing a quick update https://github.com/KyleAMathews/element-resize-event/pull/38, I wanted to find how exactly Semiotic uses the library. ResponseFrame relies on the package and there are a bunch of issues I found that needs an urgent fix:

  1. The effect needs a dependencies list, otherwise the listeners keep piling up on each render https://github.com/nteract/semiotic/blob/10af92ca96ff32688d3c9a4cdade882256cc25a3/src/components/ResponsiveFrame.tsx#L62-L85

  2. The effect does not return a callback to unsubscribe from event. Even though element-resize-event stores listeners right in the DOM node, Semiotic allows custom implementation of elementResizeEvent which might not know about this specific. https://github.com/nteract/semiotic/blob/10af92ca96ff32688d3c9a4cdade882256cc25a3/src/components/ResponsiveFrame.tsx#L69-L81

Things to do:

  1. Implement an easier resize listener (element-resize-event still attempts to use old IE-only APIs). We can potentially make use of ResizeObserver
  2. Ensure proper effect cleanup and dependencies defined

cc @emeeks

alexeyraspopov commented 2 years ago

@emeeks, here is a draft implementation that I'd make if I needed a responsive SVG element for data viz: https://gist.github.com/alexeyraspopov/5527fc528c4b834be0a443679d7b772e

willingc commented 2 years ago

This seems like a reasonable approach and worth a shot.

emeeks commented 2 years ago

This looks clean and would be a great (and modernized) replacement for the existing way ResponsiveFrame does things. I think it makes sense to implement it and I'll look into implementing it unless you're already on it.

emeeks commented 2 years ago

@alexeyraspopov take a look at #593 and let me know if I got it right.

alexeyraspopov commented 2 years ago

Fixed via #593 and released in 2.0.0-rc.19