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

[Feature]: Add zoom in/out controls to Plexus graphs #2072

Merged
merged 15 commits into from
Jan 21, 2024

Conversation

Wise-Wizard
Copy link
Contributor

@Wise-Wizard Wise-Wizard commented Dec 25, 2023

Resolves #2056, which reported a concern related to [Feature]: Add zoom in/out controls to graphs #2056.

Changes Made:
In order to address the mentioned issue and enhance user experience, the following changes have been implemented:

  • Added zoom in/out controls to the graphs.
  • Implemented logic in TraceGraph and DAG to implement the Zoom Functionality
  • Modified the useControls method to save the selected zoom level in local storage, ensuring persistence across sessions.
  • Enhanced user experience by retaining and applying the last selected zoom level even after page reloads.

Testing:
The changes have been thoroughly tested to ensure correctness and compatibility. The testing process included:

  • Verifying the presence and functionality of zoom in/out controls on graphs.
  • Checking if the last selected zoom level persists across sessions and page reloads.
  • Unit tests for the new functionality have been added.

Checklist:
To comply with the project's contribution guidelines, the following checklist items have been addressed:

  • ✓ I have carefully read and followed the contributing guidelines.
  • ✓ I have signed all commits.
  • ✓ Unit tests for the new functionality have been added.
  • ✓ Linting and testing steps have been successfully executed: for jaeger: make lint test, for jaeger-ui: yarn lint and yarn test.

Screenshots:
Original Frame (Reset)
Screenshot 2024-01-20 100131
Zoomed In
Screenshot 2024-01-20 100146
Zoomed Out
Screenshot 2024-01-20 100159

Feel free to review and provide feedback. Contributions and feedback are highly appreciated!

Signed-off-by: Wise-Wizard <saransh.shankar@gmail.com>
Signed-off-by: Wise-Wizard <saransh.shankar@gmail.com>
@Wise-Wizard Wise-Wizard requested a review from a team as a code owner December 25, 2023 17:03
@Wise-Wizard Wise-Wizard requested review from joe-elliott and removed request for a team December 25, 2023 17:03
Signed-off-by: Wise-Wizard <saransh.shankar@gmail.com>
@yurishkuro
Copy link
Member

Always best to continue in the same PR, to keep the context.

@Wise-Wizard
Copy link
Contributor Author

Hi, I was working on integrating the modifications in Plexus Package, I thought the changes have to be made to Minimap in src package but turns out none of the changes I made were being reflected in he UI of the Graphs. I spent some time debugging it and turns out the changes should be made to the lib pacakge under plexus package and the changes are being reflected. The problem I am facing is the lib package is included in the gitignore file. Please help me, resolve this doubt because I am stuck on how to proceed next.

@yurishkuro
Copy link
Member

iiuc lib is the output of the build process for plexus, i.e. they will be reflected in the UI if you do yarn build. Unfortunately I don't know if there is a faster iteration process for Plexus - there has to be, but the original developer is no longer working on the project. It's also possible that the hot reload of Plexus was somehow broken once we switched to vite (even though hot reload of typescript code from the ui package still works - if you find a way to fix this it would be an awesome contribution on its own).

@Wise-Wizard
Copy link
Contributor Author

Yes exactly, Not only hot reload but making legitimate changes in the src package of plexus and restarting the terminal is also not reflecting the changes at all, whereas hot reload of jaeger-ui is working fine.
So, what would your recommended approach be here Sir? To fix the hot reload and figure out the bugs of plexus first and then make the changes?

@yurishkuro
Copy link
Member

If you can fix hot reload it would be awesome. It can be a separate PR from this.

@Wise-Wizard
Copy link
Contributor Author

Wise-Wizard commented Jan 4, 2024

Screenshot 2024-01-04 233740
Hi, Update I was able to fix the hot reload of a single file! Of the DAG Graph, making changes in the plexus src library is being reflected for the DAG Graph and have figured out the problem as to why the hot reload is not working.

@yurishkuro
Copy link
Member

have figured out the problem as to why the hot reload is not working.

can it be fixed for everyone?

@Wise-Wizard
Copy link
Contributor Author

Yes, I will have to make about 30 file changes wherever plexus is being used.

@yurishkuro
Copy link
Member

that sounds bad - can you explain what you found & the workaround?

@Wise-Wizard
Copy link
Contributor Author

Wise-Wizard commented Jan 4, 2024

I found out that the main faults were due to the faulty imports. I redirected the imports to plexus/src package for example import { Digraph, LayoutManager } from "@jaegertracing/plexus/src/index"; and import { TEdge, TVertex } from "@jaegertracing/plexus/src/types/index"; This solved the issue of why the changes were not being reflected.

@yurishkuro
Copy link
Member

yurishkuro commented Jan 4, 2024

I see - I think it makes sense, but at the same time perhaps the solution shouldn't be in changing the imports, but some fo the build settings. I am not a JS/TS expert, but for instance in the Go world you can have deps & imports that look like a 3rd-party dependency:

https://github.com/open-telemetry/opentelemetry-collector/blob/c64ac69dacdc9c1dc2c8c0e47e8ffdbe5464f295/otelcol/go.mod#L8

but then an override is provided to tell the build that they should actually be sourced locally from the monorepo:

https://github.com/open-telemetry/opentelemetry-collector/blob/c64ac69dacdc9c1dc2c8c0e47e8ffdbe5464f295/otelcol/go.mod#L105

@Wise-Wizard
Copy link
Contributor Author

Ohh okay, I have also worked in Go and have some knowledge about how it works. So, I understand what the requirement is. I will see if the same thing is possible for TS.

@Wise-Wizard
Copy link
Contributor Author

Yeah, it's done. Had to make changes in the package.json file which redirected it to the src package instead of the lib package, and now the hot reload is working for all the files succesfully!

@Wise-Wizard
Copy link
Contributor Author

But, noticed there are some remaining imports in files that are specifically pointing to the lib folder which must be changed, rest all pointing to the main plexus folder now points to the src instead of lib.

@yurishkuro
Copy link
Member

now the hot reload is working for all the files successfully!

awesome! Definitely submit this as a separate PR

@Wise-Wizard
Copy link
Contributor Author

Got it, will do. Do you also want me to change the specific imports in each file that point out to the plexus/lib packages as well?

@yurishkuro
Copy link
Member

yes, thanks

@Wise-Wizard
Copy link
Contributor Author

Hi @yurishkuro, I would like to resume working on this issue first. Making changes to DiGraph in Plexus library as you mentioned and also add the magnifying Icons as in https://react-icons.github.io/react-icons/search/#q=magnifying.

Signed-off-by: Wise-Wizard <saransh.shankar@gmail.com>
Signed-off-by: Wise-Wizard <saransh.shankar@gmail.com>
Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5ffd926) 96.57% compared to head (5d0ffdc) 96.57%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2072   +/-   ##
=======================================
  Coverage   96.57%   96.57%           
=======================================
  Files         254      254           
  Lines        7620     7620           
  Branches     1986     1986           
=======================================
  Hits         7359     7359           
  Misses        261      261           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

please include screenshots in the RR description

packages/plexus/src/zoom/ZoomManager.tsx Outdated Show resolved Hide resolved
packages/plexus/src/zoom/resetZoomIcon.tsx Outdated Show resolved Hide resolved
Signed-off-by: Wise-Wizard <saransh.shankar@gmail.com>
@Wise-Wizard
Copy link
Contributor Author

please include screenshots in the RR description

Included the Screenshots as well.

@yurishkuro yurishkuro added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Jan 20, 2024
Signed-off-by: Wise-Wizard <saransh.shankar@gmail.com>
Signed-off-by: Wise-Wizard <saransh.shankar@gmail.com>
yurishkuro
yurishkuro previously approved these changes Jan 21, 2024
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@Wise-Wizard
Copy link
Contributor Author

Thanks!

Don't mention it :)
Also, I would like to start contributing to Jaeger(Backend), any issues in that repo you would like me to work on?

@yurishkuro
Copy link
Member

Just ran the test, looks great, but one styling change - increase the contrast of the icons.

.u-miniMap > .plexus-MiniMap--button {
  color: #444 // was #888
}

before

image

after

image

@yurishkuro
Copy link
Member

yurishkuro commented Jan 21, 2024

Also, I would like to start contributing to Jaeger(Backend), any issues in that repo you would like me to work on?

As always, just look for help-wanted / good-first-issue tickets

Signed-off-by: Wise-Wizard <saransh.shankar@gmail.com>
@Wise-Wizard
Copy link
Contributor Author

Just ran the test, looks great, but one styling change - increase the contrast of the icons.

.u-miniMap > .plexus-MiniMap--button {
  color: #444 // was #888
}

before

image after image

Done!

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@yurishkuro yurishkuro changed the title Graph/zoom buttons [Feature]: Add zoom in/out controls to Plexus graphs Jan 21, 2024
@yurishkuro yurishkuro merged commit 4a6ca03 into jaegertracing:main Jan 21, 2024
10 checks passed
@Wise-Wizard Wise-Wizard deleted the Graph/Zoom_Buttons branch January 22, 2024 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:new-feature Change that should be called out as new feature in CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Add zoom in/out controls to graphs
2 participants