Style splits #630
Style splits #630
Conversation
VeraZab
force-pushed
the
color-splits
branch
3 times, most recently
from
051016b to
f2e321f
Compare
VeraZab
force-pushed
the
color-splits
branch
8 times, most recently
from
72f9278 to
1283e97
Compare
| excludeFits, | ||
| } = this.props; | ||
| renderGroupedTraceFolds(groupedTraces) { | ||
| if (this.props.useFullData) { |
There was a problem hiding this comment.
Let's merge useFullData and canGroup so that you don't need this if... mostly so you can remove the else case below :)
| } | ||
| }, | ||
| }; | ||
| return Object.keys(groupedTraces).map((traceType, index) => { |
There was a problem hiding this comment.
this is the "grouped but not full data" case that's never used
| } | ||
|
|
||
| renderIndividualTraceFolds(filteredTraces, filteredTracesFullDataPositions) { | ||
| if (this.props.useFullData) { |
There was a problem hiding this comment.
Good refactoring! It shows that there's basically no reuse in this function any more, so we should go the next step and split it in half: renderTraceFolds for the non-canGroup case and renderUnGroupedTraceFolds for the canGroup-but-ungrouped case.
| ); | ||
|
|
||
| if (canGroup && filteredData.length > 1 && groupedTraces.length > 0) { | ||
| if (canAdd) { |
There was a problem hiding this comment.
This is basically an early-return case that means we don't need groupedTraces so let's move it up above the computation of groupedTraces and save a bit of time when canAdd
| <TabPanel> | ||
| <PlotlyPanel> | ||
| {individualTraces ? individualTraces : null} | ||
| {filteredTraces.length |
There was a problem hiding this comment.
this ternary is reused in a few places. Let's push it into renderIndividualTraceFolds to make it a bit more robust, and then we can make this code more readable.
| <TabPanel> | ||
| <PlotlyPanel>{groupedTraces ? groupedTraces : null}</PlotlyPanel> | ||
| <PlotlyPanel> | ||
| {Object.keys(groupedTraces).length |
There was a problem hiding this comment.
let's push this ternary into renderGroupedTraceFolds
| } | ||
| ); | ||
|
|
||
| if ( |
There was a problem hiding this comment.
it feels like this should be in shame.js no? Must it really be in this file?
|
|
||
| const GraphSubplotsPanel = (props, {localize: _}) => ( | ||
| <SubplotAccordion canGroup> | ||
| <SubplotAccordion> |
| }); | ||
| } | ||
|
|
||
| render() { |
There was a problem hiding this comment.
Nice refactoring. This is a pretty hairy component and it's already looking a lot better! At some point I wonder if we can't break it apart more completely by having a GroupedTraceAccordion but not today.
|
Percy's diffs seem really odd to me here, not sure what it's doing. Locally I see Trace Opacity just fine, and I don't see weird extra padding. |
|
Only issue I see so far is:
|
src/shame.js
Outdated
|
|
||
| let indexOfSplitTransform = null; | ||
|
|
||
| graphDiv.data[traceIndex].transforms.filter((t, i) => { |
There was a problem hiding this comment.
lol, just didn't want to write a for loop.. i could write a for loop
There was a problem hiding this comment.
our linter won't complain, but generally function inside filter should return something...
There was a problem hiding this comment.
yeah you're right, changing
src/shame.js
Outdated
| function getProp(group) { | ||
| let indexOfStyleObject = null; | ||
|
|
||
| graphDiv.data[traceIndex].transforms[indexOfSplitTransform].styles.filter( |
|
just comments above, and 💃! |
|
ok, I think I've changed everything that needed to be changed on this pr. I made a codepen with the same json here: I do not see opacity in fullData, posted in plotly_js channel, might open an issue in plotly.js |
|
ok, so, from a conversation with @alexcjohnson , this seems to be the correct behaviour. " |
Closes: #528
Here are the changes that were introduced with this pr:
Folds are created based on the number of traces in fullData if the
useFullDataflag is used on a TraceAccordion: this is done in the StyleTracesPanelWhen the useFullData flag is used, a new prop
fullDataArrayPositionis passed to the Fold, which indicates the index/indexes in the fullData array, that Fold represents (depending on whether the fold represents a single trace or a group of traces).If we're using fullData in the TraceAccordion, the format of
traceIndexeschanges as well, in the case that we have split traces. Because split traces all refer to a single trace in the data array, in the individual style tabs, we will have more than one Fold that will have the same traceIndex. Also in the grouped tab, we could have the same index repeated many times.Ex:
grouped scattered traces fold, could have:
traceIndexes: [0, 0, 0, 1],
fullDataArrayPosition: [0, 1, 2, 3]
Styling split traces, writes to their data[x].transforms[y].styles[z].value object in the individual panel AND in the grouped trace panel. In the grouped case, it wasn't enough to just write the style to the parent trace because if a style had been already present in the transforms[y].styles[z].value object, then the parent could not override it.