Skip to content
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

Victory transitions aren't using datum.key #684

Closed
lapidus opened this issue Jul 29, 2017 · 10 comments
Closed

Victory transitions aren't using datum.key #684

lapidus opened this issue Jul 29, 2017 · 10 comments

Comments

@lapidus
Copy link

lapidus commented Jul 29, 2017

I am trying to get a scatter chart to animate from one dataset to another dataset.
The datasets are of different lengths but have some items in common.
I would like the Scatter chart to do the 'data join' by the 'key' property so that the correct elements are entering, exiting and merging.

[{
key: 'unique-key-1',
x: x,
y: y
} ... 10 more]

=> 

[{
key: 'unique-key-6',
x: x,
y: y
} ... 4 more]

I have tried some different approaches but the join happens by 'index' instead of 'key' no matter what I try.

From what I understand, there is some support for this?
#246
FormidableLabs/victory-core#36

Any hints appreciated! @divmain @boygirl

@boygirl boygirl changed the title Data join by 'key' not working? Victory transitions aren't using datum.key Aug 2, 2017
@boygirl
Copy link
Contributor

boygirl commented Aug 2, 2017

@lapidus I expect this to work. Can you make a reproduction in jsfiddle?

@lapidus
Copy link
Author

lapidus commented Aug 2, 2017

@boygirl:
Here is a Fiddle:
https://jsfiddle.net/n2c19j17/4/

(In this example I'm expecting the 'blue' circle to turn 'orange'. In other words, the data join should take 'key' into account and not simply use the index.)

{ x: 1, y: 2, fill: "blue", key: "c" },
{ x: 2, y: 3, fill: "green", key: "b" },
{ x: 3, y: 5, fill: "red", key: "a" }
=>
{ x: 1, y: 2, fill: "blue", key: "a" },
{ x: 2, y: 3, fill: "green", key: "b" },
{ x: 3, y: 5, fill: "orange", key: "c" }

Thanks for any clues ...
ps. I realize I can use a sortKey to sort the elements beforehand but that will not work for more complex scenarios like different counts of elements.

@lapidus
Copy link
Author

lapidus commented Aug 6, 2017

@boygirl
Super if you have a chance to look into this one. It is a bit of a dealbreaker for us if we can't do joins by key instead of index :) Let me know if I can provide any further examples. I can also Skype anytime if you want me to demonstrate the code I have and the results we are seeing :)
Thank you!

@boygirl
Copy link
Contributor

boygirl commented Aug 6, 2017

@lapidus I took a quick look. I think the issue is related to how animation is behaving when no new nodes are entering or exiting. The transitions themselves respect datum.key (as in the correct node will transition in or out) but animations do not. Currently we are using d3-interpolate for animations, and it just compares elements in order, but I can add some logic to pre-sort the data array by key.

To be clear... in the example you give, you are expecting the keyed elements to move position and change color based on their key, correct? So the element keyed "c" should move from (1, 2) to (3, 5) and turn from blue to orange?

@boygirl
Copy link
Contributor

boygirl commented Aug 6, 2017

Like so:
keyed-animation

@lapidus
Copy link
Author

lapidus commented Aug 6, 2017

Thank you!
The animation looks correct :)
Does 'sorting' have any side effects?
How is for example z-index generally determined here? (Do big radius points always end up behind smaller or can this somehow be affected by this change/PR? Just curious :) )

@boygirl
Copy link
Contributor

boygirl commented Aug 6, 2017

This change wont have any effect on render order (there is no fine-tuned z-index control, unfortunately). The render order is determined by index in your sorted data array (or by eventKey if you provide one).

The sortKey prop is also still respected even as order changes. In the gif below I've changed the sortKey to y. You can see the data re-sorting as it animates so the line is always drawn between adjacent y values

sortkey

getKeyedData() {
    return [
      { x: 1, y: 5, fill: "blue", key: 1 },
      { x: 2, y: 3, fill: "green", key: 2 },
      { x: 3, y: 4, fill: "orange", key: 3 }
    ];
  }

  getKeyedData2() {
    return [
      { x: 1, y: 5, fill: "blue", key: 2 },
      { x: 2, y: 3, fill: "green", key: 3 },
      { x: 3, y: 4, fill: "red", key: 1 }
    ];
  }

@boygirl
Copy link
Contributor

boygirl commented Aug 6, 2017

I expect this change to be included in a planned release tomorrow :)

@lapidus
Copy link
Author

lapidus commented Aug 7, 2017

@boygirl:
That would be awesome :)
In case not, is there a somewhat straightforward way to integrate this Pull Request into victory using package.json or some custom magic or would I have to build the whole thing myself? Thanks!

@boygirl
Copy link
Contributor

boygirl commented Aug 7, 2017

It's released in victory-core@17.2.0 so you should be able to pick it up with a fresh npm install of Victory (current version). victory@0.21.4 will also go out later today.

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

No branches or pull requests

2 participants