Skip to content
This repository was archived by the owner on Aug 5, 2020. It is now read-only.

Improve state transitions #13

Merged
merged 1 commit into from
May 19, 2018
Merged

Improve state transitions #13

merged 1 commit into from
May 19, 2018

Conversation

kachkaev
Copy link
Collaborator

@kachkaev kachkaev commented Aug 19, 2017

Fixes #12, reduxjs/redux-devtools-chart-monitor#17, zalmoxisus/redux-devtools-extension#269

transitions.gif

I did some refactoring of update() function and thus made state transitions a bit less crazy. Node ids are now equal to something like root|branch|subBranch|subBranch[0]|property instead of just an integer, which makes it possible to re-use circles and links correctly. Thus, adding an array item somewhere near the top of the tree does not cause a huge shuffle like this:

shuffle.gif

The animation is still not ideal in a case when an item is being added to the middle of an array. This is still visualised as last item added, i+1 ... old length items changed instead of item at i added. Persistent string ids do allow for an improvement here, but this will require a complete array diffing and using hashes instead of integers for array indexes. Not sure this is worth it, at least at this point.

A couple of other fixes:

  • text alignment is now animated (watch todoStore in the gif above)
  • node radius is being taken from options (used to be hardcoded by mistake)
  • blinkDuration was added to options

Did not upgrade to d3@4 so keep changes minimal.

Demo: https://kachkaev.github.io/d3-state-visualizer-test/
Demo repo: https://github.com/kachkaev/d3-state-visualizer-test

@@ -18,7 +18,7 @@ const defaultOptions = {
collapsed: 'lightsteelblue',
parent: 'white'
},
radius: 5
radius: 7
Copy link
Collaborator Author

@kachkaev kachkaev Aug 19, 2017

Choose a reason for hiding this comment

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

The radius from defaultOptions was not used at all, see nodeUpdate.select('circle').attr('r', 7) on old line 245

@@ -44,6 +44,7 @@ const defaultOptions = {
heightBetweenNodesCoeff: 2,
widthBetweenNodesCoeff: 1,
transitionDuration: 750,
blinkDuration: 100,
Copy link
Collaborator Author

@kachkaev kachkaev Aug 19, 2017

Choose a reason for hiding this comment

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

This value was hardcoded on old line 256

/*eslint-enable*/

function update(source) {
function update() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to pass source any more, because transition positions are now taken from previousNodePositionsById / nodePositionsById

@kachkaev
Copy link
Collaborator Author

kachkaev commented Aug 19, 2017

ping @romseguy :–)

@zalmoxisus can you please check if all works in redux-devtools-extension by doing npm link? I'm especially interested in knowing that clicking on text for 'zooming in' to the given node is not broken.

Really looking forward to see smooth transitions in the Chart tab! Using your extension nearly every day! 🥇

@romseguy
Copy link
Collaborator

@kachkaev thank you very much for this PR. As of now, I have no time to review. Maybe @zalmoxisus or somebody else can confirm everything works fine with your changes? Then I'll merge and release it.

@kachkaev
Copy link
Collaborator Author

kachkaev commented Aug 30, 2017

Thanks for your comment @romseguy. @zalmoxisus seems to be busy these days too, but says he is open to move his devtools-related repos to a new org called redux-devtools. Details here: zalmoxisus/redux-devtools-extension#269 (comment). What do you think of moving the chart monitor and this repo there too so that more people could help with PRs?

I'm not pretending to become a part of that group because I'm more or less a stranger, but I'm sure you'll find a few enthusiastic folks that have enough time and trust. And the wheels will start rolling again.

@romseguy
Copy link
Collaborator

romseguy commented Aug 31, 2017

Looks like a good idea to me. I don't have experience with Github organizations so let me know how I should go about it I guess 🤔

@kachkaev
Copy link
Collaborator Author

I guess that after @zalmoxisus creates a new org and makes you a member of it, you'll be able to go to your repo's settings and transfer it from there. The old urls will redirect to a new place, so no troubles for googlers or old blog posts!

If you know anyone else who's been an active contributor to any redux-devtools parts, it'd be great if you also suggested @zalmoxisus to add them. The more trusted people are members of the groups, the higher the probability that the new PRs are merged quicker. I've seen this working in a couple of places around github quite well.

@kachkaev
Copy link
Collaborator Author

kachkaev commented Sep 20, 2017

Hi again @romseguy. I understand that you are busy, just curious if you'd be interested in finding time to move the project to a group.

@zalmoxisus I saw you started milestone 3.0 for redux-devtools-extension, which sounds like great news! Would you be happy to create a new group for the project and accept d3-state-visualizer and redux-store-visualizer to it as a part of your path to 3.0? This may release more of @romseguy's time and help grow the community around your awesome repos. WDYT?

UPD: Just noticed that milestone 3.0 was around for a few months, but it's still great to see some movement in the repo.

@romseguy
Copy link
Collaborator

romseguy commented Sep 27, 2017

if you'd be interested in finding time to move the project to a group.

I am, just waiting for @zalmoxisus to create the org.

@kachkaev
Copy link
Collaborator Author

ping @zalmoxisus

1 similar comment
@kachkaev
Copy link
Collaborator Author

ping @zalmoxisus

@kachkaev
Copy link
Collaborator Author

Given that we're still waiting for an org, would you mind reviewing and merging this @romseguy?

@romseguy
Copy link
Collaborator

romseguy commented Feb 6, 2018

I have no time / will to maintain this anymore and it looks life you dived pretty deep in the project, I could give you push access to this repo if you'd like to.

Thank you for the beautiful PR btw :)

@kachkaev
Copy link
Collaborator Author

kachkaev commented Feb 6, 2018

No worries that you can no longer support the repo – thanks for creating it in the first place! I'll be happy to help as long as I can.

I'll also ask @gaearon if we can move all redux-devtools repos to an org in the near future. Will share a link to an issue once I create it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nodes are not reused properly when adding/removing them
2 participants