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

Alerting: Indicate panels without identifier #74746

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

gillesdemey
Copy link
Member

What is this feature?

Panel IDs are optional. This wasn't taken in to account in the DashboardPicker and when opening the selector and choosing a dashboard that had any panels without id the front-end would crash.

The typings were both incorrect and also not correctly applied (dashboardDTO.panels is typed as any 😖)

This change shows the panels without identifiers but disables it from being selected and adds an indicator that those cannot be selected.

@gillesdemey gillesdemey added this to the 10.2.x milestone Sep 12, 2023
@gillesdemey gillesdemey requested a review from a team as a code owner September 12, 2023 14:58
@gillesdemey gillesdemey requested review from VikaCep, konrad147 and soniaAguilarPeiron and removed request for a team September 12, 2023 14:58
@@ -73,11 +74,12 @@ export const DashboardPicker = ({ dashboardUid, panelId, isOpen, onChange, onDis

const filteredPanels =
dashboardResult?.dashboard?.panels
?.filter((panel): panel is PanelDTO => typeof panel.id === 'number' && typeof panel.type === 'string')
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the filter here, omitting the panels from the selection seemed confusing for the user

Copy link
Contributor

@VikaCep VikaCep left a comment

Choose a reason for hiding this comment

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

The solution works great!

I'm just wondering, does it make sense that a panel ID is optional though? Why wouldn't a panel have an ID?

@gillesdemey
Copy link
Member Author

I'm just wondering, does it make sense that a panel ID is optional though? Why wouldn't a panel have an ID?

It doesn't to me, no. However it seems like the API does allow dashboards to be created with panels that have no identifiers so we just have to roll with it :D

Copy link
Contributor

@konrad147 konrad147 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@soniaAguilarPeiron soniaAguilarPeiron left a comment

Choose a reason for hiding this comment

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

💯

@gillesdemey gillesdemey merged commit 999aa41 into main Sep 13, 2023
27 checks passed
@gillesdemey gillesdemey deleted the alerting/fix-optional-panel-id branch September 13, 2023 10:35
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 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
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants