zooniverse / front-end-monorepo

A rebuild of the front-end for zooniverse.org
https://www.zooniverse.org
Apache License 2.0
105 stars 29 forks source link

Plotting graphs in React - a discussion #135

Closed rogerhutchings closed 5 years ago

rogerhutchings commented 6 years ago

TESS will require us to build an interactive light curve viewer. How are we going to do this?

TL,DR: We use vanilla React and D3, hooking them up with react-faux-dom

Summary from chats with @shaunanoordin:

However, via https://oli.me.uk/2015/09/09/d3-within-react-the-right-way/, we came across https://github.com/Olical/react-faux-dom. This provides a fake dom that can be rendered by react, but that has an DOM-like API for D3 to manipulate.

rogerhutchings commented 6 years ago

cc @chrislintott @trouille @srallen @eatyourgreens

trouille commented 6 years ago

Really appreciate you posting these notes. Will be good to post updates as you learn more about react-faux-dom.

srallen commented 6 years ago

I'd be concerned with performance of yet another virtual DOM layer.

The main problem is that the React-D3 implementations available aren't well maintained, and despite how stable and functional they are right now, I'm afraid this might bite us in the long term.

There are many implementations out there. It just the top google hit for 'react d3' that is deprecated. Anyone take a look at https://formidable.com/open-source/victory/ for instance?

srallen commented 6 years ago

Just to clarify my questions/comments is that I'm not against the virtual DOM approach on principle or anything, I just would like to see some prototyping of it and I'm not convinced of it being a better approach than a fully realized react implementation of the D3 library.

srallen commented 6 years ago

@shaunanoordin I know you'll likely be busy with catching up from being out for most of this week, but could you think about trying a prototype with the proposed libraries in this issue in the next couple of weeks? I'm about what the performance is like with two virtual DOMs running.

shaunanoordin commented 6 years ago

Can do, but I'll only able to start next week (Mon 24 Sep 2018). 👌

srallen commented 6 years ago

@shaunanoordin I saw a demo at the conference I attended of a neat performance auditing tool that we can use to help us understand how this library is doing: https://developers.google.com/web/tools/lighthouse/

It's built in to chrome dev tools, so should work well there. The top question is whether or not there's a performance hit for subjects with a lot of data points and finding out if there's a cut off that we should communicate back to the research team.

shaunanoordin commented 6 years ago

Ah, butts. I need to scrap the D3.js + React Faux Dom prototype 💀 since it breaks down when I try to make the charts interactive.

Full details:

This one's on me, as I should have fully digested the documentation before trying to build the fun stuff.

screen shot 2018-10-09 at 15 12 58 https://github.com/Olical/react-faux-dom#limitations

🤦‍♂️

rogerhutchings commented 6 years ago

:(

For what it's worth, I was planning to build panning and zooming into the subject viewer's parent, since that will also have the interaction and annotation layers. But that wouldn't work for e.g. folding axes...

srallen commented 6 years ago

Ah yeah, it didn't occur to me that that would be an issue, but makes sense with the separate virtual DOM. Thanks for investigating this.

@shaunanoordin have you had a chance to explore Victory.js? https://formidable.com/open-source/victory/

I also think we should still possibly consider Plot.ly's React charting library too, since Plot.ly has a robust number of tools like DASH (python generated React components) and their in browser UI for chart building that researchers might like.

Either option means full integration of D3's API into React which will be limiting for future use. I can discuss this with @trouille today.

I don't think we should try to build our own API to go between React and D3, personally.

shaunanoordin commented 6 years ago

@rogerhutchings we defo need to talk about this on Thu. I'd like to know more about the Subject Viewer, so I'd know where the pan/zoom controls would be built into (e.g. should it be handled by the Light Curve Viewer, or should we pass transforms into the LCV component as a prop.)

@srallen Victory is now on my list of prototypes to build. ✌️

I tried building with Plotly previously, but decided it was too opinionated on how it designed its UI and UI interactions before jumping into D3-React. That said, I didn't spend enough time trying to customise it, so I'll put that back on my prototype list.

screen shot 2018-10-09 at 15 57 27 Early Plot.ly test using plotly.js-dist + react-plotlyjs. Setting up the data is easy and zoom/pan comes built in, but I didn't figure out in time how to disable the toolbar, or to disable the more obnoxious interactions such as drag-click = zoom on target, double-click to un-zoom

srallen commented 6 years ago

I'd like to know more about the Subject Viewer, so I'd know where the pan/zoom controls would be built into (e.g. should it be handled by the Light Curve Viewer, or should we pass transforms into the LCV component as a prop.)

I think this highly depends on whether or not the zoom data needs to be in the classification metadata. We add scale/zoom level data into the classifications for images since reasonably zoom could potentially affect classification quality. I'm not sure if this was done with Planet Hunters or if the researchers for TESS would care about that. If it doesn't need to be in the classification metadata, then we could keep zoom function for data plots self contained to the component itself or use built in function if we go with a fully integrated React and D3 plotting library.

noraeisner commented 6 years ago

I think it would be useful to have the zoom data in the classification metadata, however, if putting that in is going to be extremely tricky then we can definitely live without it.

shaunanoordin commented 6 years ago

Some quick notes about using the victory library:

Before I proceed with further prototyping work using this lib, I'm going to spend a bit more time to see if there's something that can be fixed in regards to versions 30.0.0+ - notably, I'll see if I can install victory on my instance of the monorepo, to see if it's just my CFE setup that's not playing nice with the library.

srallen commented 6 years ago

@shaunanoordin Huh, maybe it's a webpack or babel configuration issue. Just to try it out myself, I installed and and was able to load up the bar chart from victory.js in the current monorepo app-project. I could not do it in zoo-reduxify; I saw the same error. I tried updating all of the core packages for babel and webpack (see https://github.com/zooniverse/zoo-reduxify/pull/27) and I'm still seeing the same error. I'm not seeing yet what the issue is yet, but it does seem isolated to zoo-reduxify.

rogerhutchings commented 6 years ago

Just to go back to D3 quickly, a random thought occurred to me earlier: do we really need the chart to interact with React?

I'd need to research this a bit more thoroughly, but theoretically, we'd have the subject and annotation stores available as Observables to interact with. If we can pass in references to them when instantiating a D3 object in the viewer component, then D3 can react to changes in the store without worrying about React at all.

All React would then have to do is mount the component creating the chart, and tidy everything up on unmount. We can prevent rerenders by always returning false from shouldComponentUpdate.

It does hinge on whether D3 can do anything clever with Observables, so like I said, more research needed. But it might be a way forward...?

srallen commented 6 years ago

This article discusses the various ways to integrate D3 with React: https://www.smashingmagazine.com/2018/02/react-d3-ecosystem/

The most important point out of this article is:

They should never share DOM control. That would be a recipe for disaster.

If it was a static plot, then I think I would be more comfortable with the idea of bypassing React and have D3 write to the DOM directly, but since there are requirements for interactivity, I think we must err on the side of having React control the DOM.

I think we should narrow our scope for prototyping and resolve on a solution soon. I don't think we should use vanilla SVG or D3, or a DIY D3 to React API at all. Reason being is that I don't think we have the developer time for it either now or for future maintenance. I spoke with @trouille yesterday and the idea that researchers may want to extend a D3 visualization themselves is not a critical requirement. Ultimately I think tools like DASH or Bokeh are powerful enough that researchers could implement what they'd like to see from the JSON subjects directly at their own discretions using python.

Outside of Victory.js or Plot.ly's libraries, both of which are based on D3, there are several others that we could look into that are also listed in the above article.

rogerhutchings commented 6 years ago

@shaunanoordin and I have just had a talk about where he's at, and he should be able to get Victory.js up and running in the classifier now. He's going to try prototyping with it tomorrow, and should have a better idea of what it can do by the end of Monday, which is when I'd like to make a decision.

I do want to keep the option of using vanilla D3 open though. Any option we choose is going to have some development burden; if we choose Victory, we'll still need to do some reasonably complex data manipulation - on the subject data itself, rather than the chart object - in order to provide axis folding, since that isn't something Victory (or likely any other charting library) supports out of the box. I'm guessing D3 will have some tools to at least simplify that process.

It's probably a lack of imagination, but I'm also not understanding how there will be any clashing if we allow D3 to handle its own interactivity.

srallen commented 6 years ago

With the increased flexibility for the timeline of the beta, @rogerhutchings, @shaunanoordin, and I discussed the following next efforts:

We will discuss again on Monday and come to a decision on what to use. If neither Victory.js or Plot.ly meets requirements, then the fallback is vanilla D3.js likely using some method of overriding the React Component class lifecycle methods.

trouille commented 5 years ago

End result of tests: using vanilla D3.js. See https://github.com/zooniverse/front-end-monorepo/blob/master/docs/arch/ard-6.md

rogerhutchings commented 5 years ago

cc #190 and #197