search

nteract / semiotic

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

Update ResponsiveFrame #593

Closed emeeks closed 2 years ago

emeeks commented 2 years ago

Tear out element-resize-event per @alexeyraspopov's example gist. Seems to work great.

vercel[bot] commented 2 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nteract/semiotic/GRqUJgwqHZtmnbWioQZHxb8DSM9z
✅ Preview: Failed

[Deployment for 85b92e4 failed]

alexeyraspopov commented 2 years ago

LGTM. Tests need a fix, like this one but it can be fixed later.

There are a couple of things that worth discussing, given this change:

  1. elementResizeEvent prop becomes obsolete, and removing it should probably be reflected in changelog (if the prop has ever been documented)
  2. ResizeObserver has decent browser support but it might not be good enough if Semiotic is considered to be supported by wider range of browsers. Do we explicitly state which browsers are supported, e.g. IE11? The polyfill for ResizeObserver exists, but it would be better to know in advance if we need to mention this explicitly in docs.
  3. Semiotic has ResponsiveXXX version of some XXX components. Do you think it would be reasonable to figure a way to use composition (e.g. <ResponsiveFrame><NetworkFrame /></ResponsiveFrame) so users can just wrap a viz piece instead of replacing the target component they use? This would reduce the API surface area, at least. Not sure about other implications right now
emeeks commented 2 years ago

LGTM. Tests need a fix, like this one but it can be fixed later.

There are a couple of things that worth discussing, given this change:

  1. elementResizeEvent prop becomes obsolete, and removing it should probably be reflected in changelog (if the prop has ever been documented)
  2. ResizeObserver has decent browser support but it might not be good enough if Semiotic is considered to be supported by wider range of browsers. Do we explicitly state which browsers are supported, e.g. IE11? The polyfill for ResizeObserver exists, but it would be better to know in advance if we need to mention this explicitly in docs.
  3. Semiotic has ResponsiveXXX version of some XXX components. Do you think it would be reasonable to figure a way to use composition (e.g. <ResponsiveFrame><NetworkFrame /></ResponsiveFrame) so users can just wrap a viz piece instead of replacing the target component they use? This would reduce the API surface area, at least. Not sure about other implications right now
  1. I don't think it was ever documented, it was only used for testing.
  2. I spent a lot of effort to make Semiotic v1 backward compatible to some older versions of IE. I do not think it's necessary to do so with v2. Not sure how best to highlight any issues to devs.
  3. This seems doable.