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

NodeGraph: Allow to set node radius in dataframe #74963

Merged
merged 1 commit into from Sep 25, 2023

Conversation

piggito
Copy link
Contributor

@piggito piggito commented Sep 15, 2023

What is this feature?

Allow to have an optional nodeRadius column in nodes dataframe to control node sizes.

Why do we need this feature?

Currently all nodes have a fixed radius of 40 and different node radius could improve visualization.

Who is this feature for?

Admin users managing node graph

Which issue(s) does this PR fix?:

Fixes #74966

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@piggito piggito requested review from a team and imatwawana as code owners September 15, 2023 15:36
@piggito piggito requested review from sunker, joshhunt and tskarhed and removed request for a team September 15, 2023 15:36
@CLAassistant
Copy link

CLAassistant commented Sep 15, 2023

CLA assistant check
All committers have signed the CLA.

@grafana-pr-automation grafana-pr-automation bot added type/docs pr/external This PR is from external contributor area/frontend labels Sep 15, 2023
Copy link
Collaborator

@imatwawana imatwawana left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! The update to the docs looks good. This just needs technical review now.

@aocenas
Copy link
Member

aocenas commented Sep 19, 2023

Hey @piggito, thanks for the PR, this look like a good feature and the code looks good so far but I am seeing some compilation issues. Are you able to fix those so we can test this?

@piggito piggito force-pushed the expose_node_size branch 2 times, most recently from 7be9ed1 to ffd2e27 Compare September 19, 2023 15:55
@piggito
Copy link
Contributor Author

piggito commented Sep 19, 2023

hi @aocenas , thank you for your feedback.
I fixed the failing tests please can you recheck the PR?

@aocenas
Copy link
Member

aocenas commented Sep 22, 2023

Hey @piggito thanks for fixing those. Trying to test it now but when I try our test data source there are bunch of errors where it seems the edge is not computed correctly. I assume there is some issue with the node radius not being defaulted somewhere and thus creating NaNs. The node radius should be optional and so it should still work the same if it's not passed in the data frame.

On main:
Screenshot 2023-09-22 at 14 58 56

On this branch:
Screenshot 2023-09-22 at 15 03 17

Error:
Screenshot 2023-09-22 at 15 06 17

@piggito
Copy link
Contributor Author

piggito commented Sep 22, 2023

@aocenas I fixed the NaN errors, please can you have another look at it?

@aocenas aocenas added this to the 10.2.x milestone Sep 25, 2023
@aocenas
Copy link
Member

aocenas commented Sep 25, 2023

@piggito looks good now. Thank you very much for the contribution 👍

@aocenas aocenas merged commit ffb15ef into grafana:main Sep 25, 2023
16 of 17 checks passed
rwwiv pushed a commit that referenced this pull request Oct 2, 2023
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NodeGraph: Allow to set node radius in dataframe
5 participants