-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[DO NOT MERGE] slider-generating transform #1099
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
Conversation
@etpinard I can't quite figure out the correct sequence. At the moment, you have to double click to relayout the plot before the slider will show up. I think I need to similarly readjust the sequence of events and pull a couple tricks in order to get defaults supplied and the slider options filled in correctly. I'll update this with my analysis as I work through it:
Also: It seems like it will be very difficult to get this not to fail in the case where two traces talk to the same slider and have different |
Clearest case that is weirdly ambiguous:
|
Have you tried putting everything in a (regular supply-defaults) |
@etpinard I split half (slider setup) into |
@etpinard @chriddyp Here's a first cut a working slider-generating transform: The input: https://github.com/rreusser/animation-experiments/blob/master/src/plots/gapminder-simplified.js Features to notice:
|
From a data point-of-view, I think that non-ambiguous way this would be represented would be with the group-by transform, i.e. "one trace" with
|
Looks like you've figured out the ambiguity case though (by taking the union of the target data?). Looks really good! 🎉 |
@chriddyp The specific problem is that accomplishing this as data transforms leads to a need to specify layout-type config on multiple data traces. Short of building a concept of layout transforms that I think is best avoided for now, I don't see any way around that. As long as the resulting syntax stays straightforward, the current form is really just about the best I can do. I'm worried that shorthand notation like I'm working on tests now, but if this works for you, the steps the front end will need to accomplish will be:
To elaborate slightly, I totally get the desire to boil this down to a couple keywords, but I think we're a long way away having the features, the implementation, and lots of other details locked down well enough to take that approach. For example: it's not clear to me how we'd pass animation config or slider config with a single keyword, and it seems like a very bad idea to add all of the necessary keywords to the trace itself. |
Hey @rreusser - Do you have an update on this one? |
@jackparmer I had a long agonizing talk with @etpinard yesterday about the conclusion that there's no way to make this feature reasonably robust with transforms as they currently exist. Transforms aren't written in a way that can handle layout components in any reasonable manner. He pushed this branch which adds some additional capability to transforms. I'm rewriting now in a way that should remove 80% of the complexity. I'm not 100% convinced it'll be trivial now, but it should be at least an order of magnitude (or two?) easier. Will update with progress ASAP, but I expect that after two full rewrites already, a day should be adequate here. |
@etpinard I've completely rewritten this transform based on yesterday's discussion. Once it was implemented correctly, all of the existing tests passed with only a small naming change! 🎉 One thing I couldn't get working: when all transforms are deleted, the module is no longer in the and so the transform doesn't get run to clean up after itself. This is probably a secondary concern, but other than that, I feel not so bad about this. Again, it's an order of magnitude easier than what I was doing before. cc @chriddyp |
Oh, and I should mention: I implemented it in this repo since it was much easier to develop here. Streambed is up and running for me though, so should be easy to pick this up and drop it in that repo. I do need to separate out the additional supplyLayoutDefaults step for transforms though. I'll create a separate PR for that. |
Next steps for finishing this off:
See example at (play button needs naming fix in order to work): |
I can take care of
if you want. |
@etpinard That's okay with me since you know obscure concerns better than I do. If so though, note the extra changes required to pass |
@etpinard Current state: I'm happy with how this is working. There are a couple prerequisite PRs still making their way through. Moving this code to streambed is easy. Blocking: the version of static plotly.js in streambed. Is updating straightforward? |
transferred to https://github.com/plotly/streambed/pull/8428 |
This is a rough cut at a transform that generates a slider based on filter transform input.