Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Remove Event system #433

Closed
alexcjohnson opened this issue Jan 9, 2019 · 3 comments
Closed

Remove Event system #433

alexcjohnson opened this issue Jan 9, 2019 · 3 comments

Comments

@alexcjohnson
Copy link
Collaborator

Companion issue to plotly/dash#531

Events are used only lightly in this repo. The changes I see to remove them completely:

  • Graph has events, and corresponding properties with the data, for things like click events, hover events... I don't see any need to use the events instead of just letting the property trigger the callback, but folks using the events would need to alter their code. Do we need to add timestamps for these, or is the existence of the data properties enough?

  • A bunch of components expose change events, fired with every character typed for example. The underlying value will also trigger a callback, I believe in all of these cases, so I don't think we're losing any functionality, but we may want to add timestamps - to all core components in fact - to help distinguish most-recent-user-input, for cases of multiple parallel ways to update the same output, as described in Remove Event system dash#531

  • Input already has n_blur[_timestamp] and n_submit[_timestamp] with no corresponding events. It doesn't even have a change event, but the note above ^^ still may apply to it.

  • Interval currently fires an event and increments n_interval - so we simply need to remove the event. No timestamp property is provided, would be an easy add if anyone was interested.

  • Textarea currently exposes blur and click events, which need to be updated to properties.

@Marc-Andre-Rivet
Copy link
Contributor

Seems like the only places we’re a bit fuzzy on the impact is dcc.Graph and some other dcc components that use the change event and that we may want to check behavior w/ props — talked a bit with Nicolas about the Plotly.js React wrapper before the holidays and there was mention of the graph modifying certain value that React would have considered immutable, wondering if we will run into issues where the event is the only way, as-is, to get the trigger correctly / consistently?

@alexcjohnson
Copy link
Collaborator Author

there was mention of the graph modifying certain value that React would have considered immutable, wondering if we will run into issues where the event is the only way, as-is, to get the trigger correctly / consistently?

I believe what @nicolaskruchten is talking about there falls into two categories:

  • Changes during initial plotting, like filling in auto values - range when autorange is true, for example. These would have no events associated with them anyway, this is just something we have to clean up on the plotly.js side.
  • Mutations due to user interaction, like zooming. These are the cases we currently have events for, but they also all have corresponding properties that change to reflect the updates. So I don't think there's any missing functionality if we remove the events, in fact the properties are much more useful as they contain the full information about what happened in the event. That said, there are two extra pieces we may want to address in the future:
    • There doesn't seem to be an event or property associated with plotly_restyle events. These can occur via gui for example when selecting/deselecting traces in the legend.
    • I don't believe there's a way to have a callback retrieve the actual figure present in the Graph component, as mutated by the user, or any part of that figure. You can try to piece that together from the plotly_relayout (and plotly_restyle) events, but that's a tricky prospect, and anyway this won't give changes from the initial plotting. Perhaps even better would be if there was some way to access pieces of _fullData and _fullLayout - then you could do things like pull out automatic trace colors or bin sizes to share with other components.

Anyway these are all possible extensions that occur to me as I'm looking at the current state, not anything that would be lost by removing events and would need replacing.

Re: change events - we could certainly sprinkle n_changes[_timestamp] properties around liberally, to give folks a 100% turn-the-crank upgrade path. I'm still not sure it's necessary, but I wouldn't be opposed out of the abundance of caution.

@Marc-Andre-Rivet
Copy link
Contributor

All good by me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants