[WIP] Trace picker updates #311

aulneau · 2018-02-07T19:39:44Z

Okay!

So this is not quite ready yet. I've built out the views and I built out a little modal provider that we can use with other modals if we want to keep it.

General structure of the updates:

I've added two new props to PlotlyEditor:
advancedTraceTypeSelector which enables everything, and traceSelectorConfig which passes our default configs to the enhanced trace type selector. The default config values are coming from some constants I ported over from streambed which you can see here. Let me know if this data structure should change.

I have created a ModalProvider.js which passes two functions through context to anywhere in the application: openModal(component) and closeModal(). We call openModal and pass it a component -> see here.

I have also created a Modal.js component that we can pass params to. Currently it accepts a title param for the header, and then we can pass it content via children wrapped in <ModalContent>...</ModalContent> I might just bake this in rather than using a sep wrapper component for this. The Modal.js takes care of animating in/out.

Where I need help // cc @VeraZab @nicolaskruchten
I am not exactly sure the best way to hook up the TraceTypeSelector.js to be able to affect the larger application state (change chart types, only allow X type charts if Y type chart is already selected (see here), etc. Currently I am passing the same props that we pass to the drop down to the TraceTypeSelector.js, but I will need some guidance moving forward.

Things I still need to do:

Hook up the enhanced picker to actually change the trace type
Convert the new chart type images to react svg components (and add the last 2 from @fabric-8)
Provide a way for people to pass in custom images along with their custom data
Possible default image/icon for a chart type that is passed but does not have a plotly image / provided image
Allow item actions to be customizable

Will resolve: #238

VeraZab · 2018-02-07T19:59:17Z

Convert the new chart type images to react svg components

do you mean by this: put these icons into the plotly-icons repo?

aulneau · 2018-02-07T20:00:53Z

@VeraZab possibly -- these svgs are a lot more complex (various strokes, different colors) -- but it might make sense to add them there. I'll have to either manually make them into components or adjust our generating scripts for them.

VeraZab · 2018-02-07T20:42:53Z

To make the Modal change the plot, you have the updateContainer prop, you can pass it the value of the chart that the user clicks on. So for ex, for a timeseries chart, we have to make sure the timeseries value is passed on. It's a custom type. Careful with the custom types. We have that function plotlyTraceToCustomTrace to make those conversions, it takes container as an argument (gd.data).

VeraZab · 2018-02-07T20:43:29Z

but it might make sense to add them there.

yup, I think it does make sense to add them there, that repos purpose is really to take care of all our icons

nicolaskruchten · 2018-02-07T20:44:48Z

A few comments:

Looks great!
regarding the 2d/3d chart exclusion, basically in this component there needs to be some check of what's currently in the figure, and then some decision about which chart types to grey out. We can implement this after the basic version is done in a separate PR, but I don't see this as "affects the larger application state" so much as "conditionally renders based on state" if you see what I mean...
(Vera beat me to it)

VeraZab · 2018-02-07T20:47:08Z

src/DefaultEditor.js

- <StyleColorbarsPanel group={_('Style')} name={_('Color Bars')} />
- </PanelMenuWrapper>
+ <Fragment>
+ <PanelMenuWrapper>


is Fragment needed there?
If I understand correctly, Fragment's used to group elements in place of wrapping them in a div, but I don't see why it'd be useful here..?

Whoops that's left over from when I was exploring ways of adding the modal :)

VeraZab · 2018-02-07T20:49:59Z

src/PlotlyEditor.js

 import {bem} from './lib';
 import {noShame, maybeClearAxisTypes} from './shame';
 import {EDITOR_ACTIONS} from './lib/constants';
 import isNumeric from 'fast-isnumeric';
 import nestedProperty from 'plotly.js/src/lib/nested_property';
+import {CATEGORY_LAYOUT, TRACE_TYPES} from 'lib/constants';
+import {ModalProvider} from 'components/containers';



we have a modal system in webapp, and I think that with a modal system being present now in the editor, this just means that both modal systems will have to coexist. Because the biggest thing in webapp with Modals was that we wanted to have Modal action chains.

One modal appears, then based on user's action, another one displays. But I think it's ok not to worry about it too much here. I think that what will most likely happen is that there's 2 modal systems that will exist.

Yeah I was curious about that, I figured that in this version of the editor we didn't need chained modals, but I think we could merge it into one when the time comes :)

VeraZab · 2018-02-07T20:53:14Z

src/components/fields/TraceSelector.js

+ this.context.openModal(<TraceTypeSelector {...props} />)
+ }
+ >
+ <UnconnectedDropdown {...props} />


Just from a UI and visual standpoint, maybe it should look more like a button here, instead of an UnconnectedDropdown?

Cool -- I will build out a button similar to what is in the workspace currently :)

nicolaskruchten · 2018-02-07T20:54:08Z

re how to wire things in to the bigger context, take a look at how the other widget/field pairs work.

basically your TraceTypeSelector is an unconnected widget, and the TraceSelector is a field which connects it. So the field will pass you a prop of your choice bound to its props.updatePlot which you need to call whenever the user makes a choice.

Edit: right now TraceSelector cheats because it grabs UnconnectedDropdown from fields instead of widgets, so a better pair to look at is Numeric/NumericInput :)

VeraZab · 2018-02-07T20:55:35Z

src/components/widgets/TraceTypeSelector.js

+ aria-label="View tutorials on this chart type."
+ data-microtip-position={`top${position}`}
+ role="tooltip"
+ >


ok, you'd add the tutorial link later? just a reminder :) it should be an a tag?
and actually, I think this text _('View tutorials on this chart type') has to be localized.

There's a few places in here where text needs to be localized : )

Yeah, I think I am going to abstract these out anyway to they are a part of the config that a user can pass through... enable them, change them, etc

VeraZab · 2018-02-07T20:55:46Z

src/components/widgets/TraceTypeSelector.js

+ <div
+ className="trace-item__actions__item"
+ aria-label="See a basic example."
+ data-microtip-position={`bottom${position}`}


VeraZab · 2018-02-07T20:59:36Z

src/lib/constants.js

+ STATISTICS: 'STATISTICS',
+ MAPS: 'MAPS',
+};
+


Could these structures be simpler, like:

export const CHART_CLASS = { GEOGRAPHIC: ['atlas', 'choropleth', 'scattermapbox'] }

CHART_CATEGORY is needed for the picker and CHART_CLASS for compatibility and disabling and enabling different plots. So I see why both are needed. But I don't see why we should repeat that whole object that lists all chart types..

VeraZab · 2018-02-07T21:00:07Z

src/lib/constants.js

+};
+
+export const CATEGORY_LAYOUT = [
+ {category: CHART_CATEGORY.BUSINESS, label: 'Business'},


These Category layouts need to be localized

VeraZab · 2018-02-07T21:01:08Z

src/lib/constants.js

+
+export const TRACE_TYPES = {
+ scatter: {
+ meta: {


this is kind of repeating, with more detail, what we have here:
https://github.com/plotly/react-plotly.js-editor/blob/master/src/components/fields/TraceSelector.js#L21

I don't see it's necessity

Much of these consts were pulled from streambed I was not sure how much of it should stay / how much of it should go.

What do you think about moving https://github.com/plotly/react-plotly.js-editor/blob/master/src/components/fields/TraceSelector.js#L21 into the constants.js file?

We don't have to use what streambed has : ) we just need the logic (basically which chart types go into which categories). Plus our new requirement now is that users can make that configurable. So the best data structure is the one that's simplest to use in an API.

About https://github.com/plotly/react-plotly.js-editor/blob/master/src/components/fields/TraceSelector.js#L21 I think they're here because trace names need localization, and that localize function must wrap a component to make the localize prop available to it.

We should probably be making localize available at the very top level of our app, localizing component per component is a bit rough and unecesary..

I don't mind moving all trace types to the constants folder though..

there should definitely be one canonical mapping from trace type to localized trace name, so that if I have access to "scatter" and _ I should be able to get at the localized name. Makes sense to move that into constants and pack in the default categorization as well IMO.

VeraZab · 2018-02-07T21:01:37Z

src/styles/components/containers/_modal.scss

@@ -0,0 +1,99 @@
+.modal {
+ $c: &;
+ position: fixed;


is this a forgotten variable?

VeraZab · 2018-02-09T15:05:59Z

hey @aulneau!
we've discussed this a little bit here: #260 (comment)

And finally decided not to put these as separate trace types into the chart picker: 'error bars', 'animations', 'timeseries'. Would you remove them from the picker, please.

aulneau · 2018-02-09T15:48:38Z

Okay I've started going through and creating lists based off of the icons that Fab has made, and the list I pulled from streambed, and the list contained here in this editor and this is what I've got so far:

Trace types with category and icon -> here

Trace types with icon but no category -> here

Trace types with no icons and no category -> here

Also @VeraZab and @nicolaskruchten do we want to couple the category type in with the trace type, eg:

A

    {
        value: 'scatter',
        label: _('Scatter'),
        category:CHART_CATEGORY.BUSINESS,
    },

or do we want to keep trace types clean and do something like:

B

    {
        value: 'scatter',
        label: _('Scatter'),
    },
---
const categories = [
  {
    value: 'statistics',
    label: _('Statistics'),
    traces: ['scatter', 'line', ...more],
  },
  ...
];

I am leaning more towards A.

nicolaskruchten · 2018-02-09T15:51:29Z

I like A, it's DRYer

aulneau · 2018-02-09T16:48:02Z

Okay -- I have added all the trace types that have icons into categories.

cc @VeraZab @nicolaskruchten @jackparmer for any suggestions on moving trace types to a more appropriate category:

nicolaskruchten · 2018-02-09T16:59:18Z

Hmmm. How attached are we to our existing categorization scheme, @jackparmer ? It seems a bit limiting at this point if we're adding all the other trace types...

Maybe we want to use a more function-oriented approach like this? https://datavizcatalogue.com/search.html or https://visual.ly/community/infographic/how/graphic-continuum

There are bunch of other organization types here https://github.com/widged/data-for-good/wiki/Visualisation-::-Choosing-a-chart

jackparmer · 2018-02-09T18:38:05Z

Hmmm. How attached are we to our existing categorization scheme

Not attached to it at all. I'm sure y'all can come up with something much better.

Edit: I think the "functional" categorization is too high brow though. Is there a 7th categorization to take the weight off of the "Science" column? Maybe a "Heatmaps" column where we put heatmap, countour, 2d histogram, and 2d histogram contour.

There's also a bit of shuffling to do in the current screenshot: carpet and sankey should be under "Science". parcoords I think should be under "Stats"

nicolaskruchten · 2018-02-09T19:05:06Z

Here's a slightly different typology which includes all 3 carpet types and might fit into a 4-row/8-column grid.

Simple (8, first row)
- Scatter
- Bar
- Line
- Area
- Heatmap
- Contour
- Table
- Pie
3D (5, second row)
- 3D scatter
- 3D line
- 3D surface
- 3D mesh
- Pointcloud
Maps (3, second row)
- Choropleth
- Satellite
- Atlas
Distributions (5, third row)
- box
- violin
- histogram
- 2d histogram
- 2d contour histogram
Finance (2, fourth row)
- OHLC
- Candlestick
Specialized (7, fourth row)
- Ternary
- Polar
- Parallel Coordinates
- Sankey
- Carpet
- Scatter Carpet
- Carpet Contour

jackparmer · 2018-02-09T19:14:32Z

I like it! Only thing is pointcloud isn't 3d. I think we should have a 7th column called "2d WebGL" that has scattergl, pointcloud, scatterpolargl, and heatmapgl. This will be good for us - it's too hard to play around with the WebGL trace types currently.

nicolaskruchten · 2018-02-09T19:18:21Z

I was thinking that the GL types would be like a checkbox or something rather than a first-class citizen in this trace-picker. The user looking at this might not want to make the "underlying technology" choice at this point...?

What's pointcloud if not 3-d?

jackparmer · 2018-02-09T19:23:31Z

+1 Checkbox/switch sounds great to me. Maybe on the top-right here? One side of the switch says D3/SVG and the other side says WebGL?

What's pointcloud if not 3-d?

This magical trace type that @monfera wrote for uber refresh rates with lots of 2d scatter data:

https://codepen.io/monfera/pen/BLjJVZ
https://plot.ly/javascript/pointcloud/

If users want to plot a stupid # of x-y points interactively, we point them to pointcloud

nicolaskruchten · 2018-02-09T19:27:28Z

One downside to the SVG vs WebGL thing at the trace-picker level is that then 3D would need to go under WebGL, right?

jackparmer · 2018-02-09T19:28:41Z

I would label it "2d WebGL" and keep 3d charts under both views

nicolaskruchten · 2018-02-09T19:32:39Z

OK so my initial thinking about the checkbox was outside of this modal. If we want to have the GL versions on this modal, I would go with a separate section called "WebGL accelerated" or something with those trace types in there.

jackparmer · 2018-02-09T19:35:11Z

👍 "WebGL accelerated" column. I don't have a strong preference about the WebGL display as long as they are easy to access like the other trace types.

VeraZab · 2018-02-20T15:14:17Z

src/components/fields/TraceSelector.js

+ }
+ if (
+ plotlyTraceToCustomTrace(context.container) === 'timeseries' &&
+ (!context.layout.xaxis ||


oh.. we removed this code.. maybe time for a little rebase?..

VeraZab · 2018-02-20T15:15:04Z

src/components/widgets/TraceTypeSelector.js

+ href: `#`,
+ icon: <GraphIcon />,
+ },
+];


yeah, it's ok, we can add these in another pr : )

VeraZab · 2018-02-20T15:21:54Z

src/lib/computeTraceOptionsFromSchema.js

+function computeTraceOptionsFromSchema(schema, _, context) {
+ // Filter out Polar "area" type as it is fairly broken and we want to present
+ // scatter with fill as an "area" chart type for convenience.
+ const traceTypes = Object.keys(schema.traces).filter(


so, when is this code activated now?
When advancedTraceTypeSelector is not provided as a prop to the PlotlyEditor Component?
I think the user should still be able to configure the plot types even if they don't choose the advancedTraceTypeSelector.

So this should be adjusted and only read from schema if a trace config was not given.
And these should be the excluded types for now: 'area', 'violin', 'pointcloud', 'parcoords', 'sankey', 'carpet', 'scattercarpet', 'contourcarpet', 'scatterpolar'

VeraZab · 2018-02-20T15:24:11Z

src/lib/customTraceType.js

@@ -25,6 +25,8 @@ export function traceTypeToPlotlyInitFigure(traceType) {
 return {type: 'scatter', mode: 'markers', fill: 'none'};
 case 'area':
 return {type: 'scatter', fill: 'tozeroy'};
+ case 'cartesianArea':
+ return {type: 'scatter', fill: 'tozeroy'};


why was cartesianArea added?,
and btw, these:
https://github.com/plotly/react-plotly.js-editor/blob/caf87d269d8bc270054bae0fc99b66bed464f64f/src/lib/customTraceType.js#L8

have to match these:
https://github.com/plotly/react-plotly.js-editor/blob/caf87d269d8bc270054bae0fc99b66bed464f64f/src/lib/customTraceType.js#L26

So area's already in there, we don't need 'cartesianArea'

Will remove :) it was held over from when I ported some stuff over from streambed

VeraZab · 2018-02-20T15:24:46Z

src/lib/traceTypes.js

+ label: _('Heatmap GL'),
+ category: chartCategory(_).WEB_GL,
+ },
+];


great : ) thanks!!

VeraZab

ok, well, as this was almost ready for a rereview, except for new icons, I think it needs a few readjustments as per comments, but looks great, and almost ready!! 🎉 thanks @aulneau !

VeraZab · 2018-02-20T16:00:53Z

I think I'd vote here to make these adjustments, and merge this pr in. We have all the icons for the currently integrated traces in the editor. We can make smaller prs to integrate the rest, and make layout improvements as we go.

nicolaskruchten · 2018-02-27T20:01:25Z

dev/App.js

@@ -83,6 +83,7 @@ class App extends Component {
 dataSources={dataSources}
 dataSourceOptions={dataSourceOptions}
 plotly={plotly}
+ advancedTraceTypeSelector


Do we need this flag? this is basically going to be the default config right?

ignore this comment, i have no objection here :)

VeraZab · 2018-02-28T22:02:49Z

@aulneau checked out your branch and played around with it, looks good! let's merge!
and then when all icons are in plotly-icons we can make those changes in another pr. thanks!! 🎉

VeraZab · 2018-03-03T03:09:14Z

ok, I think I resolved all rebase issues here, going to merge and if anything was missed then we'll see pretty soon :)

VeraZab reviewed Feb 7, 2018

View reviewed changes

nicolaskruchten mentioned this pull request Feb 8, 2018

Grouped Trace tabs #312

Merged

VeraZab reviewed Feb 20, 2018

View reviewed changes

src/lib/traceTypes.js Outdated

label: _('Heatmap GL'),

category: chartCategory(_).WEB_GL,

},

];

Copy link

Contributor

VeraZab Feb 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great : ) thanks!!

VeraZab suggested changes Feb 20, 2018

View reviewed changes

nicolaskruchten reviewed Feb 27, 2018

View reviewed changes

VeraZab approved these changes Feb 28, 2018

View reviewed changes

VeraZab force-pushed the trace-picker-updates branch 2 times, most recently from 064d0a9 to dddd62b Compare March 3, 2018 02:51

aulneau and others added 14 commits March 2, 2018 22:09

added hot loading, built out new tracepicker

ffb56b9

refine modal, add tunnels

d42ea4f

update modal provider, customize tooltips

2899960

add link to examples

0ee5001

update modal provider to accept props

2c8b661

abstract out computTraceOptions

1ade2b0

rename icons, update categories and arrangement

014bee7

clean up, refine layout, abstract out actions, misc updates

c82d00c

change from select to custom button for advanced trace selector

e32c830

update adv trace selector button, other misc cleanup

bda60a9

cleanup for tests

30efa1c

fixes via vera, other tweaks

128bf83

bump plotly-icons version

2aa2bba

Rebase

391957d

VeraZab force-pushed the trace-picker-updates branch from dddd62b to 391957d Compare March 3, 2018 03:10

VeraZab merged commit e47aae5 into master Mar 3, 2018

VeraZab deleted the trace-picker-updates branch March 3, 2018 03:15

[WIP] Trace picker updates #311

[WIP] Trace picker updates #311

Conversation

aulneau commented Feb 7, 2018 • edited Loading

VeraZab commented Feb 7, 2018 • edited Loading

aulneau commented Feb 7, 2018 • edited Loading

VeraZab commented Feb 7, 2018

VeraZab commented Feb 7, 2018 • edited Loading

nicolaskruchten commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaskruchten commented Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeraZab commented Feb 9, 2018

aulneau commented Feb 9, 2018

nicolaskruchten commented Feb 9, 2018

aulneau commented Feb 9, 2018

nicolaskruchten commented Feb 9, 2018

jackparmer commented Feb 9, 2018 • edited Loading

nicolaskruchten commented Feb 9, 2018

jackparmer commented Feb 9, 2018

nicolaskruchten commented Feb 9, 2018

jackparmer commented Feb 9, 2018 • edited Loading

nicolaskruchten commented Feb 9, 2018

jackparmer commented Feb 9, 2018

nicolaskruchten commented Feb 9, 2018

jackparmer commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeraZab left a comment

Choose a reason for hiding this comment

VeraZab commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeraZab commented Feb 28, 2018

VeraZab commented Mar 3, 2018

aulneau commented Feb 7, 2018 •

edited

Loading

VeraZab commented Feb 7, 2018 •

edited

Loading

aulneau commented Feb 7, 2018 •

edited

Loading

VeraZab commented Feb 7, 2018 •

edited

Loading

nicolaskruchten commented Feb 7, 2018 •

edited

Loading

jackparmer commented Feb 9, 2018 •

edited

Loading

jackparmer commented Feb 9, 2018 •

edited

Loading