-
Notifications
You must be signed in to change notification settings - Fork 627
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
Initial support for linked views and animation #554
Conversation
The demo does not work for me on Windows 7/Chrome Version 49.0.2623.110. Am I misunderstanding? I'll dig into the code and update Chrome. There are no JavaScript console errors. |
The brushing has no effect for me either. Again, no js errors in console. |
Sorry, forgot to mention, you have to click the little dotted box outline in the plotly toolbar first to put it in selection mode instead of zoom mode. I couldn't figure out how to make this the default. |
That makes sense, and I thought I tried, but I guess I didn't try hard enough. It is working. |
@jcheng5 |
Updated the demo to default to select mode, and I'm clearing selection boxes now (by hacking on the DOM directly, can clean this up if plotly.js adds an API for this--I need to file an issue). |
@jcheng5 is it possible for me to add commits to this pull request? I just tried: git fetch origin pull/554/head:crosstalk
git checkout crosstalk
git add *
git commit -m "add skip_on_pull_request()"
git push origin crosstalk:joe/feature/crosstalk but it just created this new branch. If it isn't possible, that'd be great if you could send a pull request from the ropensci remote (you should be a collaborator now). |
Ok @cpsievert, I added you. |
@jcheng5 I like what you've done in 7ec2ec3 to allow different "sets" of traces. As you probably know, the Before anyone puts more work in, let me outline my vision for how linked selections could work more generally (feel free to chime in @etpinard, @alexcjohnson, @timelyportfolio): In plotly.js, most (if not all) trace types support a constant
We could then allow users to define the "selection"/"deselection" events and the opacity multiplier from |
I agree that
|
I don't really understand the subplots vs. Would the approach you outlined (adding a separate trace when something is selected, and decreasing opacity on the original trace) work for a pie chart? How about a stacked bar or area chart (if those even make sense for linked brushing)? |
Sorry, I was mistaken, using |
I suppose for something like pie charts, overlaying the filtered data wouldn't work, but would require overlaying all the data and hiding graphical elements with Another problem is that, for some trace types (e.g., heatmap),
I'd have to think about this, but as you show in your example (in the initial comment), it is certainly useful to overlay bars/densities related to the selection (which can only be done client-side by adding another trace). This gets into the realm of non 1-to-1 mappings in the source view <-> data <-> target view pipeline, which seems like a huge messy problem, but it'd be awesome if one day something like this could replace your example. subplot(
ggplot(mtcars, aes(wt, mpg)) + geom_point(),
ggplot(mtcars, aes(wt, disp)) + geom_point(),
ggplot(mtcars, aes(cyl)) + geom_bar(position = "identity"),
crosstalk = ct_opts(key = row.names(mtcars))
) Layering selection traces would also have the advantage of supporting a sequence of selections which is super useful for comparisons, for example: That being said, I don't want to open pandora's box and prevent progress from being made. Let's just focus on reducing the opacity of non-selected points for now, and leave trace layering for another pull request. |
@cpsievert Speaking of Pandora's box, I'm now looking at adding rudimentary client-side filtering support (for plots without aggregated data). It doesn't look to me like One brute-force option I have is to essentially just re-run renderValue, should I just do that? |
I'm pretty sure the brand spanking new filter transforms will be helpful for fully hiding/removing points. Personally, I find highlighting (rather than fully removing) to be more useful, so this isn't a huge issue for me, at least initially. I'm currently flirting with the idea of manipulating the DOM directly with |
@cpsievert The transform plugins look cool. It looks to me, though, like it's basically a more declarative way to transform data on the way into the plot--I could just do that transformation myself. The part I'm less sure about is how to trigger the plot to redraw when the filtering changes, short of calling Aaaand I just discovered |
@cpsievert I was able to get everything I needed (AFAICT) with Just a word of warning, in case you have local changes based off this branch: I had to completely blow up and refactor the existing code to get this working. The JS code for the htmlwidget binding is getting big enough that we should perhaps start considering using modules and unit tests, and ES2015 (or TypeScript if you prefer). If you're up for that in principle, I can implement it (maybe not this week but soon). |
I do have local changes, but I was anticipating conflicts and started a new branch, so no worries. BTW, I ditched the idea of manipulating the DOM with D3js. I'm back to what I outlined here. I could definitely use some help with filtering, so I'm looking forward to seeing what you have thus far! I agree that we'll need some JS unit tests, but I don't have much experience with this, any opinions on this @timelyportfolio? Should we aim for consistency with the plotly.js approach? |
@cpsievert, you read my mind. I think consistency with I am trying to think through the best way to run these tests from |
Oh, I wasn't actually thinking we'd run the tests from R. I was thinking we'd just run the JS tests from the command line (as well as browserify). Jasmine/karma is fine. |
@cpsievert, @etpinard should we tie in continuous with |
If at all possible I'd prefer to keep the entire testing suite on TravisCI |
// Preserve the original data. We'll subset based off of this whenever | ||
// filtering is (re)applied. | ||
this.origData = JSON.parse(JSON.stringify(graphDiv.data)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcheng5 just getting back to this now after a major overhaul of subplot()
...can't wait to dig in and help out!
First question: how come we need JSON.parse(JSON.stringify())
here? To verify it's valid JSON?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to me like the purpose of this is a deep copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It's a bit questionable to do it this way as it loses any functions, Date objects, and any other custom objects that aren't JSON, but it didn't look to me like the .data
property would have any of that anyway...?
@jcheng5 with |
The easiest, but I'm not sure best way, is to add the |
One idea I had a long while back, but never got around to doing it is a |
I've done this already to some extent within rmarkdown, right now here is html_dependency_jquery These could of course easily be moved to a new package called perhaps We'd also need as Ramnath pointed out a syntax for referencing these in On Fri, May 20, 2016 at 11:45 AM, Ramnath Vaidyanathan <
|
@jjallaire, I use the
|
@jjallaire i like the name On the question of syntax for referring to these, I was thinking
By default, a user need not specify This would call for a little bit of change in |
I'd call it htmldependencies rather than htmlwidgetdeps because these can
On Fri, May 20, 2016 at 12:04 PM, Ramnath Vaidyanathan <
|
…ce() because of group2NA()
… itself rather than the user
Lots of work still needed here, but it works enough that you can get a sense of what I'd like to see happen here. This is similar to other demos that have existed in the past, but unlike my previous demos it now uses plotly, not just ggplot2/ggvis/base graphics; and unlike @cpsievert's previous demos (to my knowledge) it now does client-side linked brushing between two distinct plotly plots.
Live demo:
https://beta.rstudioconnect.com/jcheng/shiny-crosstalk/
crosstalk
param?)--this should ideally be consistent across all Crosstalk-compatible htmlwidgets (Update: key/group are attributes of thecrosstalk::SharedData
object whichplot_ly()
/ggplotly()
will require for enabling linked views)plotly_selecting
and you get a really nice experience--but to make it not overwhelm the server we need to useplotly_selected
right now. Maybe should formalize this in Crosstalk and have both "fast" and "slow" subscription models.cc @jjallaire, @ramnathv
@hafen, I'm hoping to look more closely at rbokeh next.