Skip to content

Conversation

@dmt0
Copy link
Contributor

@dmt0 dmt0 commented Jul 9, 2018

plotly.js version bump to 1.39.1

Since plotly.js no longer copies UID from _fullData to data, we can't rely on UID to identify traces and from now on have to use fullData[i]._fullInput.index, which will correspond to index in data.

@dmt0 dmt0 self-assigned this Jul 9, 2018
@dmt0 dmt0 requested review from VeraZab and nicolaskruchten July 9, 2018 18:34
@dmt0 dmt0 force-pushed the version-bump-1391 branch from 4f1816a to d66a6a9 Compare July 9, 2018 18:35
}
if (
isNumeric(payload.transformIndex) &&
payload.traceIndex < graphDiv.data.length
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not sure I understand this line: payload.traceIndex < graphDiv.data.length

before we used to loop and find the trace in data that matched our transform index. how is this replaced now with the line above?

do we not need to loop, like we used to but have the comparison be:
if (payload.transformIndex === graphDiv._fullData[i].index) in place of if (graphDiv.data[i].uid === payload.traceUid)

Copy link
Contributor Author

@dmt0 dmt0 Jul 9, 2018

Choose a reason for hiding this comment

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

I figure we looped in order to find the UID, now we go by index. Why did we loop in the first place, when we could just do graphDiv.data.find(...)?..

And perhaps we don't need the line that you mentioned either. I just put it there defensively.

Copy link
Contributor

Choose a reason for hiding this comment

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

payload.transformIndex, indicates a _fullData index?

indexes are not 1:1, an index in data could have a different index in _fullData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

plz check the discussion in plotly_js slack:

nicolas [13:40]
we're trying to match up from data not fullData

etienne [13:41]
fullData[i]._fullInput.index will be the index in gd.data
nicolas [13:42]
ok so we might be able to go from if (trace.uid === fullData[i]._fullInput._input.uid) {
to if (theIndex === fullData[i]._fullInput._input.index) {
assuming theIndex is the position of trace in the data array?
etienne [13:44]
yeah, that should work

Copy link
Contributor

Choose a reason for hiding this comment

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

ok sure, yes them it looks like you don't need that conditional,
thanks!

let fullTrace = {};
for (let i = 0; i < fullData.length; i++) {
if (trace.uid === fullData[i]._fullInput._input.uid) {
if (traceIndexes[0] === fullData[i]._fullInput.index) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be traceIndexes[i] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a traceIndexes array for each trace, presumably to identify traces that you get when you split or something like that? So seems like traceIndex[0] always points to THE trace. Or am I misunderstanding something?

Copy link
Contributor

@VeraZab VeraZab Jul 9, 2018

Choose a reason for hiding this comment

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

traceIndexes is an array of either:

  • one single trace (like [0])
  • or an array of traces ([[0, 1]]).

It will be an array when we're dealing with grouped traces, like in the accordions that allow you to style multiple traces at once.

Copy link
Contributor

@VeraZab VeraZab Jul 9, 2018

Choose a reason for hiding this comment

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

💃
i'll recheck on my fits integration branch that this works ok for fits, but in principle, I am ok with this change

@dmt0 dmt0 merged commit f624fb8 into master Jul 9, 2018
@dmt0 dmt0 deleted the version-bump-1391 branch July 9, 2018 21:23
@dmt0 dmt0 mentioned this pull request Jul 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants