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

Animate default edges in resources explorer #1142

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

jolo-dev
Copy link
Contributor

Problem

#1071

Solution

The edges are now animated.

Changes Made

  • Added Cytoscape Layers(Plugin) which allows to add animation to current Cytoscape.
  • Clean Up the Typescript script

How to Test

[Provide instructions on how to test the changes you made, including any relevant details like configuration steps or data to be used for testing.]

Screenshots

Screen.Recording.2023-10-27.at.16.27.10.mp4

Notes

  • When going from Explorer to Dashboard Home, then there is an error. I think it's a React router thing and it could solve itself when building.

Checklist

  • Code follows the contributing guidelines
  • Changes have been thoroughly tested
  • Documentation has been updated, if necessary
  • Any dependencies have been added to the project, if necessary

Reviewers

@[username of the reviewer]

@Traxmaxx
Copy link
Contributor

hey @jolo-dev
Thanks a lot for your PR! 🙌 🔥
Allie will be back towards the end of the week. I will have a look at the code soon!

@Traxmaxx
Copy link
Contributor

Hey, it looks really great so far.
I would make the animation way slower. it looks very hectic when dealing with a huge dependency graph.

I also found a but when changing the filter. It crashes with the following error for me
Screenshot 2023-10-31 at 17 21 26

Another thing I noticed is that the performance took another hit. That might be something we should look into as well 🤔

How do you experience the performance so far? You could mock-duplicate the data returned from the API to test a bigger infrastructure. Have you got it working similar to now the dashed border was built just with gradients instead? I was wondering if it's just not possible or if I did something wrong

@jolo-dev
Copy link
Contributor Author

jolo-dev commented Nov 1, 2023

@Traxmaxx Yes, the animation is configurable. You can increase the modeValue which shall apply to the duration of each animation. You can try 1.

Yeah, there is an interesting thing when changing pages or using components... Could that be a react thing where we have to use sort of a hook?

Performance-wise, I did not notice anything. Maybe my dataset was too small..

@Traxmaxx
Copy link
Contributor

Traxmaxx commented Nov 1, 2023

Performance is a bit cumbersome to test atm. Will see if I can find a way in the future to make it easier.
The filter change bug though needs to be fixed. I don't know yet why it's happening.

@jolo-dev
Copy link
Contributor Author

jolo-dev commented Nov 1, 2023

Performance is a bit cumbersome to test atm. Will see if I can find a way in the future to make it easier. The filter change bug though needs to be fixed. I don't know yet why it's happening.

Let me investigate a bit. It's really weird...

@Traxmaxx
Copy link
Contributor

Traxmaxx commented Nov 2, 2023

@jolo-dev thank you! I hope I can make up some time soon to also help out. I'll keep you posted! We will figure this out, sooner or later 😅

@jolo-dev
Copy link
Contributor Author

@Traxmaxx I am not a big React dev nor a fan :D
The problem is that the function animateEdge is not unmounted when routing to a different path.
I know you can do it with useEffect. Maybe you have an idea here?

@jolo-dev
Copy link
Contributor Author

The reason why this happens is because the function animateEdges will not get destroyed when moving to a different page. That leads to not finding the edges

@Traxmaxx
Copy link
Contributor

Traxmaxx commented Nov 20, 2023

@jolo-dev I'll take a look when I have some time. Thanks for reporting your findings!

@jakepage91
Copy link
Contributor

Hey there @Traxmaxx were you able to look into this further?

@jakepage91 jakepage91 self-requested a review December 19, 2023 10:50
Copy link
Contributor

@jakepage91 jakepage91 left a comment

Choose a reason for hiding this comment

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

This looks good to me, but yeah, let's make the live edge movement slightly slower.

@Traxmaxx
Copy link
Contributor

Traxmaxx commented Jan 2, 2024

Hey there @Traxmaxx were you able to look into this further?

Not yet since I was doing the audit pages. How fast do you need input? : )

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.

3 participants