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: Support icons for nodes #60989

Merged
merged 13 commits into from Mar 1, 2023
Merged

NodeGraph: Support icons for nodes #60989

merged 13 commits into from Mar 1, 2023

Conversation

aocenas
Copy link
Member

@aocenas aocenas commented Jan 4, 2023

Allow specifying an icon for node data that will then be used inside the node instead of the stats shown currently:

Screenshot from 2023-02-23 11-50-06

At this moment only icons already specified inside Grafana are allowed so no custom icons.

Stats are now shown also in a context menu so if icon is shown they are not totally lost.

Screenshot from 2023-02-23 11-18-32

There are some additional small changes:

  • The context header items are shown in a table layout.
  • Context menu is also shown for edges whether there is a data link or not.

Screenshot from 2023-02-23 11-43-25

Future improvement not done here: We don't handle the units of the stats properly atm so this should be fixed using Grafana standard utils.

@ryantxu
Copy link
Member

ryantxu commented Jan 5, 2023

I'm not sure if there is a way to set this in the UI... but we do have an editor that will let you pick/search icons:

https://github.com/grafana/grafana/blob/use-enum-type-in-testdata/public/app/features/dimensions/editors/ValueMappingsEditor/ValueMappingsEditor.tsx#L75

<ResourcePicker
                    onChange={(icon) => onChangeIcon(icon, rowIndex)}
                    value={row.result.icon}
                    size={ResourcePickerSize.SMALL}
                    folderName={ResourceFolderName.Icon}
                    mediaType={MediaType.Icon}
                    color={row.result.color}
                  />
                 ```

@aocenas
Copy link
Member Author

aocenas commented Jan 10, 2023

@ryantxu thanks, this though will come handy later. My assumption is that first we may implement some automatic icon mapping per data source (like x-ray probably know what is what). Then we may think about some user config to map based on some rules (probably matching node names) that could add to or override what data source defines in the response

@grafanabot
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@grafanabot grafanabot added the stale Issue with no recent activity label Feb 10, 2023
@grafanabot grafanabot removed the stale Issue with no recent activity label Feb 23, 2023
@aocenas aocenas marked this pull request as ready for review February 23, 2023 10:51
@aocenas aocenas requested review from a team as code owners February 23, 2023 10:51
@aocenas aocenas requested review from andresmgot, joshhunt and JoaoSilvaGrafana and removed request for a team, andresmgot, joshhunt and JoaoSilvaGrafana February 23, 2023 10:51
@aocenas aocenas added this to the 9.5.0 milestone Feb 23, 2023
@aocenas aocenas added no-backport Skip backport of PR add to changelog labels Feb 23, 2023
Copy link
Contributor

@joey-grafana joey-grafana left a comment

Choose a reason for hiding this comment

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

Mostly UI/UX suggestions (prob ok if didn't fix em all). Overall, love this.

Screenshot 2023-02-24 at 16 27 22

  • Only display e.g. 'Requests per second' if there is a value for it
  • Line height between 'Service name' & 'Average response time' is different to line height between 'Average response time' & 'Requests per second' (prob happening because there is no value)

Screenshot 2023-02-24 at 16 02 09

  • Hovering over edge shows blank line if no secondary stat i.e. blank line below 13.32 in image
  • Title/subtitle case: could prob capitalise here
  • Maybe a little more padding for title/subtitle/total time/self time container. So they align better with 'Open in Explore' indentation - although 'Open in Explore' has a different purpose (this is subjective really so whatever change or not you want to make here)

Screenshot 2023-02-24 at 15 54 40

  • Should we remove the bottom border

Screenshot 2023-02-24 at 16 05 08

Also

  • Could update test data source node graph scenario to include node with icon

Comment on lines +115 to +117
| Field name | Type | Description |
| ------------- | ------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| title | string | Name of the node visible in just under the node. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Could prob improve the formatting here. There's a lot of ----- and seems | does not need to be pushed to a new line.

Copy link
Member Author

@aocenas aocenas Feb 28, 2023

Choose a reason for hiding this comment

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

For me, the IDE does this automatically so that the table is aligned if the source is shown without line wraps, like

|----------|---------------------|
|content   |some more things here|

And here some of the descriptions are long so it aligns length to that.

public/app/plugins/panel/nodeGraph/Node.tsx Outdated Show resolved Hide resolved
@aocenas
Copy link
Member Author

aocenas commented Feb 28, 2023

@joey-grafana

Only display e.g. 'Requests per second' if there is a value for it

Done

Hovering over edge shows blank line if no secondary stat i.e. blank line below 13.32 in image

Done

Title/subtitle case: could prob capitalise here

This is based on displayName of a field if it's not set it uses the field name. Not sure if it makes sense to capitalize in that case, would probably keep the distinction so it's clear it's a field name and let users name it as they want.

Maybe a little more padding for title/subtitle/total time/self time container.
Should we remove the bottom border

Agree but seems like that is all in the ContextMenu component so will do a separate PR as that probaly needs review from user essentials.

Could update test data source node graph scenario to include node with icon
There should be already but it's probabilistic like ~20% of the nodes should have an icon right now.

@aocenas aocenas changed the title NodeGraph: support icons for nodes NodeGraph: Support icons for nodes Mar 1, 2023
@aocenas aocenas merged commit 78b39bb into main Mar 1, 2023
@aocenas aocenas deleted the aocenas/nodegraph/icons branch March 1, 2023 15:02
ryantxu pushed a commit that referenced this pull request Mar 2, 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.

None yet

4 participants