esbullington / react-d3

Modular React charts made with d3.js
https://reactiva.github.io/react-d3-website/
MIT License
1.75k stars 179 forks source link

Click handler for treemap #214

Closed ejaxon closed 9 years ago

ejaxon commented 9 years ago

I am hoping to use react-d3 in an updated implementation of http://www.avlbudget.org and, in particular, to use the treemap for http://www.avlbudget.org/expenses. To do it, I need to pass in a click handler that will let me know when the user clicks on a treemap cell (and of course some indication of which cell). The handler will just dispatch an event that will change the starting point in the hierarchy, which will then cause the component that uses the treemap to be re-rendered.

The project is https://github.com/DemocracyApps/GBE. The React front end is here: https://github.com/DemocracyApps/GBE/tree/master/gbe/resources/assets/js/components.

I'm on a tight deadline (May 26, 2015) - happy to do the work & issue a pull request, but could use a little guidance in doing it.

MatthewHerbst commented 9 years ago

I wonder if it might be worth wrapping all event handlers into some type of master handler, and passing that down to all charts via the mixin - that way you could pass custom functions for click, hover, etc.

Anyways, I like this idea a lot. It makes the graphs much more interactive.

What type of guidance do you need that we can give to help you do this!?

yang-wei commented 9 years ago

I need to pass in a click handler that will let me know when the user clicks on a treemap cell

Hey @ejaxon , actually if you clone the latest master you will get see the treemap is actually implemented with animation. If you look at here, you can see how we handle onMouseOver and onMouseLeave event.

Hope that this clears thing out

MatthewHerbst commented 9 years ago

@yang-wei he wants to have his code listen for clicks on the treemap. The current implementation just runs our fairly crappy animation code - we should make it so that you can override all event call backs if desired, or at least, allow for event callbacks to hit the user's code before we run the animation.

ejaxon commented 9 years ago

@MatthewHerbst, @yang-wei, Well, one mystery solved anyway. I installed via npm, but was looking at the latest master and so I was confused by the fact that I couldn't turn on the animation. Only occurred to me this morning that the reason it wasn't working was that the npm version doesn't have that yet. Duh.

Anyway, yes, I did see the implementation of onMouseOver and onMouseLeave. It should be easy just to add another property to pass down with callbacks for the various events. I started to implement for onClick, but you're right @MatthewHerbst that one might as well generalize to all events. There are a few questions for a fully general implementation, esp. how do you indicate what was clicked, ideally in terms of the data rather than the visualization object. I'm happy to contribute to the general problem, but over the next week or two will have to focus just on getting my specific case done (this is one of many things that I need to get done over the next week).

As to guidance, I'm probably mostly just dealing with my own incompetence. I cloned the repo and seemed to successfully run npm install and gulp build, but got errors when I tried to use (errors that that I did NOT get when I used via the npm installation). When I get to the office this AM I'll start over and try to do it a bit more systematically. I've only recently started working with Node and React (about a month ago) and I'm hardly an expert with Javascript either, so I'm prone to silly errors and gaps, especially with my usual "first implement then read the documentation" approach. If anyone is around today or tomorrow for questions, I'd appreciate it (I'm U.S. eastern time zone).

yang-wei commented 9 years ago

we should make it so that you can override all event call backs if desired, or at least, allow for event callbacks to hit the user's code before we run the animation

@MatthewHerbst Yes regarding to this I want to keep the simplicity of current implementation. It's a good idea but our main focus at the time is to provide an api to render data using react and d3 quickly. If you need more, you can change the source.

Of course this is considerably in the future. Would like know more the use case of other events than obMouseLeave on treemap component. Will check @ejaxon project out to see.

ejaxon commented 9 years ago

@yang-wei There are a couple standard use cases for treemaps. You can see one of them here: http://avlbudget.org/expenses. When the data is part of a hierarchy, clicking on a cell in the treemap should allow you to descend down to the next level. The second standard use case is to have a tooltip with details of the item you are hovering over. For example, if I hover over China in the sample treemap, you might show the actual value of the population number plus other statistics.

ejaxon commented 9 years ago

The tooltip example applies to many visualizations. The hierarchy descent is more specialized, but certainly can be of interest for, e.g., pie charts.

Understand the focus on simplicity, but I would highly recommend considering a general way of allowing interactivity. The advantage of allowing users to pass in handlers is that you retain the internal simplicity while creating a nice mechanism for highly interactive visualizations. This will work especially well in the React/Flux paradigm.

yang-wei commented 9 years ago

@ejaxon Yes tooltip will be supported soon once I have time to work on this.

The clicking on cell example is a good one ! I am also planning to do such thing in my project gh-top. Sure it would be great if our library has this kind of power. What's your expectation of the api provided will be.

ejaxon commented 9 years ago

Let me think on that a bit @yang-wei. Passing an object with eventType: callback pairs seems a simple way to go about it, but the key question is what context I get with the callback. Again, it's trivial with the treemap since there's a clear, discrete piece of data associated with any click - just provide a reference to the data item, which allows the user to get back any information they might have included in the structure. Not sure that's always the case.

ejaxon commented 9 years ago

Let me know if there are particular visualizations that I should include in my thinking

ejaxon commented 9 years ago

I.e., look more closely at the implementation ... I'll think about all of them, of course.

yang-wei commented 9 years ago

IMHO instead of giving user freedom to set their callback when it's clicked, we will just set the callback to be zoom in if prop (i.e zoomOpt.zoomable is set). Something like this:

var zoomOpt = {
  zoomable: [bool]          // zoomable or not
  groupBy: [group name],    // e.g continent name correspond to our homepage treemap example
  level: [number],          // e.g 1 , 2  
 ...                        // some other props needs
}
<Treemap zoomOpt={zoomOpt}  ... />

I expect we need to do some nesting or grouping to the data prop by using d3.nest, then in the children component,

<Cell
  // props.group is a reference to data item
  onClick={ props.zoomable ? props.zoom(props.group) : null  }    
/>

prop.zoom will be pass to the top parent, data changed and treemap is rerendered.

This is just rough thinking of mine, what do you think? @ejaxon

parshap commented 9 years ago

zoomable doesn't feel like the right level of abstraction to me. I would be happier with the lower level constructs (like onClick) that enable users to build custom interactions.

yang-wei commented 9 years ago

Do you mean that we just need to provide interface like

<Treemap cellOnClickHandler={customMethod} />

Hmm... let me just write out the code and figure out how to do this. Thanks @parshap

parshap commented 9 years ago

@yang-wei yes that's the kind of API I was thinking about. I'm not sure if this is the best solution for everybody, but is how I feel I could make best use of react-d3 personally.

MatthewHerbst commented 9 years ago

@yang-wei correct me if I'm wrong, but the goal of the library is to emulate d3 on React. d3 allows full customization of all the features of a graph, including overriding and setting events. Thus I honestly don't think it should be the job of this library to do any type of animation or event handling. Rather, we just need to support allowing the users to do those things if they wish. @esbullington I think your comments are needed on this thread for what your vision is.

MatthewHerbst commented 9 years ago

IMHO, I think a good way for us to go about this would be to have an eventHandlerMixin that gets implemented by all charts. Each chart would call the appropriate methods from the mixin. That way users could safely override any event handler method simply by passing it into the chart definition as a prop.

ejaxon commented 9 years ago

As my contribution to the discussion, here's my quick-and-dirty solution for my own needs: https://github.com/DemocracyApps/react-d3/tree/master/src/treemap. The example use is here: https://github.com/DemocracyApps/GBE/blob/master/gbe/resources/assets/js/components/SimpleTreemap.js. I only implemented for the treemap for now, but implemented the slight generalization I mentioned above of passing in an eventHandlers hash rather than doing each one individually.

I'm not submitting a pull request since I think there's zero chance I've done it the way you will actually want to. Happy also to take suggestions (esp. on a better way to return context back to the user about where the event occurred). I don't really know D3 at all.

ejaxon commented 9 years ago

Incidentally, the implementation works nicely now for traversing down through the hierarchy. I'll probably add in the ability to pass in the mouseover-related event handlers so that I can do a tooltip for cells where the label doesn't fit (see issue #217 ).

yang-wei commented 9 years ago

the goal of the library is to emulate d3 on React. d3 allows full customization of all the features of a graph, including overriding and setting events.

@MatthewHerbst Sounds true but when we start earlier, what we want to achieve firstly is a graph library to help users build graph as quick as possible (e.g metrics graphics).

However since we had so many contributors and different opinion on this, we might change. And the custom event handler is really a good suggestion but it does bring in complexity for our user. (i.e they have to figure out how to do this and parent-child relationship will make callback of the events difficult)

What I can say by now until Eric is back (he's busy with his job) is to keep current implementation. We do have a lot of task stacking

ejaxon commented 9 years ago

Just a quick update. I ended up not using this library, but rather porting the treemap code from our site last year (http://avlbudget.org/expenses). That not only gives me the hierarchy navigation (which was trivial to add here), it also does nice animation while zooming in or out. While that adds nothing to the substance and usefulness of the visualization, that kind of 'sizzle' is unfortunately really important to how it is perceived by users.

In this case I'm wrapping the D3 code underneath a React component, but after I get past this deadline, I will look again at whether to finish converting what I have to purer React or coming back to React-D3.

As a general comment, I recommend being careful about distinguishing between ease of use and power of the library. It's perfectly possible to build a very powerful library with lots of possibilities while retaining the ability for a new user to get a great visualization with almost no effort. As long as you retain the "does something reasonable when all you do is give it a dataset and a container" quality, the complexity of using more advanced features remains something the user can do whenever they need to get the additional functionality and are willing to invest a little more time.

yang-wei commented 9 years ago

@ejaxon Will keep your comment as a reference. Thanks for input and good luck in your project !