Multi-value support in trace tabs #314
Conversation
src/components/containers/derived.js
Outdated
| @@ -16,11 +16,7 @@ const AxesFold = connectAxesToLayout(Fold); | |||
|
|
|||
| const TraceTypeSection = (props, context) => { | |||
| const {fullContainer} = context; | |||
| if ( | |||
| fullContainer && | |||
| fullContainer._fullInput && | |||
There was a problem hiding this comment.
I don't know why we were checking _fullInput here but it was failing in multi-valued contexts
There was a problem hiding this comment.
yeah _fullInput is where the true trace type for financial traces lie
_fullData.type for an ohlc trace will show scatter, _fullData._fullInput.type, will show ohlc
And all traces are supposed to have a _fullInput key, with type..so that's why I'm looking here
We could also look in Data.type..
But yeah, if you don't look there, it won't properly look for financials
There was a problem hiding this comment.
ah, right, I keep forgetting to check stuff with financial traces. right now they cause crashes apparently... joy!
|
|
||
| this.markerColor = nestedProperty(fullContainer, 'marker.color').get(); | ||
| this.borderWidth = nestedProperty(fullContainer, 'marker.line.width').get(); | ||
|
|
||
| if (this.markerColor === MULTI_VALUED) { |
There was a problem hiding this comment.
this is very tricky: we don't actually end up using default values here and I couldn't figure out how to do it... we use the values from the first trace. I've added a note to fit'n'finish
There was a problem hiding this comment.
well, that's what the current workspace does, sounds ok to me!
| @@ -0,0 +1,77 @@ | |||
| import {MULTI_VALUED} from './constants'; | |||
There was a problem hiding this comment.
this was moved from connectAxesToLayout basically unmodified.
|
this is great @nicolaskruchten ! 💃 once we put back _fullInput (in that comment above) |
|
well, we can't just "put it back" as it doesn't work in all cases. need to find a more general solution. Seems like in general the financial traces cause a lot of problems :( |
|
what cases does it not work for? for fin charts, the true values are in _fullInput, that has to go back. |
|
Per my comment, it doesn't work properly in grouped-trace contexts. I will fix it, don't worry, my point is I can't just undo my change :) |
|
Ah, and the crash is just another variant of issues that crop up when we delete traces. It actually happens without financial traces, I just happened to have all financial traces up when I noticed it. |
|
well, Etienne admits that they're a bit of a mess, but that's what we've got now :( |
| @@ -29,7 +29,7 @@ import {localize} from '../lib'; | |||
|
|
|||
| const StyleTracesPanel = ({localize: _}) => ( | |||
| <TraceAccordion canGroup> | |||
| <TextEditor label={_('Name')} richTextOnly /> | |||
| <TextEditor label={_('Name')} attr="name" richTextOnly /> | |||
| <TraceOrientation | |||
|
ok ready for re-review. I patched things up so that nothing is totally crazy but I've excluded the financial traces from the grouping altogether as it wasn't possible for me to find a constellation of settings that made them work correctly. |
|
(I logged this in #290 of course for another pass later) |
| fullContainer._fullInput && | ||
| props.traceTypes.includes(fullContainer._fullInput.type) | ||
| ((fullContainer._fullInput && | ||
| props.traceTypes.includes(fullContainer._fullInput.type)) || |
There was a problem hiding this comment.
@VeraZab _fullInput is not available in some code paths, so this is the compromise. Specifically, in the grouped context, the all keys starting with _ are missing from container as per the original logic from the grouped Axes. If I remove this exclusion, we end up with circular JSON objects that cannot be serialized :)
| }, {}); | ||
|
|
||
| const groupedTraces = Object.keys(tracesByGroup) | ||
| .filter(traceType => !['ohlc', 'candlestick'].includes(traceType)) |
There was a problem hiding this comment.
this is ugly but prevents really weird outcomes.
|
great 💃 |
This PR includes the change to hide text editors in multi-value contexts, and also adds support for styling text scatters, which we missed in the original passes through there :)