-
-
Notifications
You must be signed in to change notification settings - Fork 114
Fit traces adjustments #747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
592eb69 to
a44d192
Compare
|
While I figure out what's wrong with the test @dmt0 @nicolaskruchten , this is ready for review and questions. |
|
Can you drop the test plz to get the checkbox back? :) |
|
|
||
| break; | ||
| } | ||
| let fullTrace = fullData.filter(t => t && traceIndexes[0] === t.index)[0] || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this logic is in three places now, could we extract it out into a util or something?
| </TraceFold> | ||
| )); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you change this to null from []?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to avoid the .length check below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah... that was just when I was trying things to get the breaking test to work..
and I thought, well, since this function is called renderTraceFolds then it felt more reacty to return null if those shouldn't display..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hehe I don't think that feels more reacty at all :) but ok.
|
💃 |
closes: https://github.com/plotly/streambed/issues/11524
A few issues were discovered by this bug:
In Style/Traces/By Type, we'd have 1
AreaTraceFold, and setMultivaluedContainer would pick the first trace: a scatter, and compare all other traces against it to see if they have a different value. The problem would occur when it would arrive at thetransformskey, a normal scatter wouldn't have that key, and so we'd enter into a gd mutating state, and start writingMIXED_VALUESinto gd. To prevent this, simply using deepCopy.fc8f4b3: cleans up index logic in TraceAccordion. The logic here got a bit murky once we've added the ability to filter certain trace types from accordions and styling of transformed traces.
this.filteredTracesDataIndexesis always meant to indicate the position in the data array ofthis.filteredTraces, no matter their base (fullData or Data). We're usingthis.filteredTracesDataIndexesin thetraceIndexesprop on TraceFolds, and those are going to be the indexes used on gd.data to make changes to our traces._expandedIndexis what we can use on afullTraceto get its position ingd._fullData.a44d192: clear up logic in connectTraceToPlot. Reading in
_fullInputis no longer necessary. This was added at the time that financial traces were transforms and as such, did not have all of the required info in theirgd._fullDatato make all the styling widgets appear. This is no longer the case, and I think it's better to remove to simplify the logic.