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

feat: GroupTreePlot --- Refactor to typescript + better D3 and React integration + some small features #2367

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

Anders2303
Copy link
Collaborator

@Anders2303 Anders2303 commented Nov 14, 2024

Resolves #1829

  • refactor: Fully rewrites the old D3-only GroupTreePlot component to more seemlessly use both D3 and React to build the graph
    • In essence; D3 is now purely used to calculate hierarchies and node positions, whilst React handles the rendering of said nodes
    • D3 is also used to animate transitions between graph states (assisted by React to trigger the animation changes)
  • dependency: Installed the react-transition-group package to facilitate leave-enter transitions for graph elements
  • dependency: Installed the motion library to facilitate leave-enter transitions for graph elements
  • refactor: groupTreeAssembler has been replaced; it has been split it into by react components (to handle svg rendering), and the new DataAssembler class, to handle the data itself. Rewritten in proper Typescript
    • Note, the old assembler would display dummy data if an empty data tree list was fed to the constructor. The new version is more picky, and will throw an error if this is done. The Core component will instead display an error showing that the data is invalid, to make it more clear that something is wrong
  • fix: The rendered tree is now more centered in the SVG
  • fix: The tree will now fill the entire width of the SVG
  • feature/fix: Child nodes can now be minimized and expanded, as intended
  • feature: Added initialVisibleDepth; renders the tree with all nodes from the given depth collapsed
  • feature: The plot is now fully resizable.
    • An example of resizing is show in the "Resizable" Story (/?path=/story/grouptreeplot-demo--resizable)

Known issue (slight animation regression)

Because each component initializes their own enter/leave animations in their enter/leave-callbakcs, the D3 animations get slightly de-synchronized, so new/old tree edges get slightly "detached" from the tree when it grows shrinks (only during the animation). The d3 only version did not have this issue (as it set up all animations in one single update), so it's a regression.

However, this is only noticable in slower animations, and with a 200ms transition duration, it's more or less impossible to notice, so I'm leaving it unresolved for now

@Anders2303 Anders2303 added enhancement New feature or request CeeSol Task owned by Ceetron Solutions group-tree labels Nov 14, 2024
@Anders2303
Copy link
Collaborator Author

Convert this comment to an issue when this is merged
As mentioned in the PR, the tree edge animation experiences some "lag" when the tree experiences rapid updates. I believe this issue is mainly caused by the animations starting within the enter/leave callbacks. I think we should instead use the callbacks to build the "target" state for the edge (likewise with changes to width and path from external updates), then initiate d3-animation call in a singular effect-hook after the component is done rendering

@rubenthoms rubenthoms changed the title feat: GroupTreePlot --- Refactor to typescript + better D3 and React intergation + some small features feat: GroupTreePlot --- Refactor to typescript + better D3 and React integration + some small features Jan 14, 2025
Copy link
Collaborator

@rubenthoms rubenthoms left a comment

Choose a reason for hiding this comment

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

Nice refactoring and a huge improvement compared to how it was before, well done 👍

I think the file structure/names can be improved and I have a couple of other comments regarding reference-stability and use of hooks.

Also, we should check if react-transition-group is still properly maintained before we add it as a dependency.

typescript/packages/group-tree-plot/package.json Outdated Show resolved Hide resolved
typescript/packages/group-tree-plot/src/utils.ts Outdated Show resolved Hide resolved
typescript/packages/group-tree-plot/src/utils.ts Outdated Show resolved Hide resolved
typescript/packages/group-tree-plot/src/utils.ts Outdated Show resolved Hide resolved
@w1nklr
Copy link
Collaborator

w1nklr commented Jan 17, 2025

Hi.
Can you add some doc ? At least on top level classes, to explain their role...

@Anders2303 Anders2303 requested a review from rubenthoms January 17, 2025 11:34
@Anders2303
Copy link
Collaborator Author

Resolved all feedback. Swapped animation library to motion, and added missing doc comments to data-assembler.

All good now, @rubenthoms , @w1nklr ?

@rubenthoms rubenthoms requested a review from w1nklr January 20, 2025 12:08
Copy link
Collaborator

@rubenthoms rubenthoms left a comment

Choose a reason for hiding this comment

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

Nice improvements to the overall file structure and motion as a "healthier" alternative to react-transition-group 👍 Some minor issues remaining, mostly naming things. Almost ready to be merged.

Copy link
Collaborator

@rubenthoms rubenthoms left a comment

Choose a reason for hiding this comment

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

One small typo, otherwise all good from my side 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CeeSol Task owned by Ceetron Solutions enhancement New feature or request group-tree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REFACTORING]: groupTreeAssembler.js in GroupTreePlot
3 participants