-
-
Notifications
You must be signed in to change notification settings - Fork 108
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.
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
graphDiv masks hidden dependencies and complicates state management #355
Comments
You're right that the editor depends on a number of I should add, however, that there is a subtlety here which I feel mitigates some of the distastefulness of this pattern: you do not really ever need to serialize/store the DOM node or its private keys, as these can all be regenerated from The simple example editor, you can see that the initial |
FWIW this PR is an attempt to make clear what does and doesn't need to be carried around in an app's state: #384 |
The default export for this package now hides the handling of the DOM node and exposes only |
@nicolaskruchten So to expand upon my concerns expressed in plotly/plotly.js#2389
react-plotly.js-editor
has these special_fullData
,_fullLayout
, and_context
properties as hidden dependencies drawn out from thegraphDiv
prop. I can't seem to find documentation on them, and the underscores seems to hint that they're supposed to be private internal variables to Plotly. It's implied thatgraphDiv
is just supposed to be a wrapper for{ data, layout }
, but the source code and examples seem to say otherwise.But since it's all hidden away inside this DOM Element wrapper, it's never fully explained in the docs that there's something special going on, and it's very unclear how to properly handle the separation of state management (like Redux, which should not be concerning itself with passing around a DOM element), and the behind-the-scenes integration between
PlotlyEditor
and thePlot
/PlotComponent
.Maybe you'd prefer to always keep
graphDiv
as a required prop for the editor for the sake of tight integration, butdata
andlayout
(and any other relevant state information) should be separated out into dedicated props.The text was updated successfully, but these errors were encountered: