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

WIP: Implement panel title search logic #845

Closed
wants to merge 6 commits into from
Closed

Conversation

kaydelaney
Copy link
Contributor

@kaydelaney kaydelaney commented Jul 24, 2024

After trying the behavior approach and finding critical limitations(PR), such as:

  • Search by title when we had repeated panels did not work
  • Saving dashboards that had a filter by title applied, removed the panels from the Dashboard JSON.

We have decided to take another chance with implementing the search logic in the scenes library.

This PR introduced new optional properties to SceneGridLayout, the idea is to use this feature toggles in the Grafana repository behind a feature toggle

  /** Panel search */
  showPanelSearch?: boolean;
  searchString?: string;
  panelsPerRow?: number;
📦 Published PR as canary version: 5.15.0--canary.845.11012035874.0

✨ Test out this PR locally via:

npm install @grafana/scenes-react@5.15.0--canary.845.11012035874.0
npm install @grafana/scenes@5.15.0--canary.845.11012035874.0
# or 
yarn add @grafana/scenes-react@5.15.0--canary.845.11012035874.0
yarn add @grafana/scenes@5.15.0--canary.845.11012035874.0

@kaydelaney kaydelaney self-assigned this Jul 24, 2024
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Is there a way to build this as a behavior or / pluggable feature for the grid? (not sure if that is the case or if it's a good idea, but feels like a non core feature of the grid itself

@kaydelaney
Copy link
Contributor Author

Is there a way to build this as a behavior or / pluggable feature for the grid? (not sure if that is the case or if it's a good idea, but feels like a non core feature of the grid itself

I agree it really isn't a core feature. I'll try and see if I can think of a way to make it more "modular"

@kaydelaney
Copy link
Contributor Author

Closing in favor of grafana/grafana#91139

@kaydelaney kaydelaney changed the title WIP: Add support for systemPanelFilterVar/systemDynamicRowSizeVar WIP: Implement panel title search logic Sep 12, 2024
@axelavargas axelavargas added minor Increment the minor version when merged release Create a release when this pr is merged labels Sep 23, 2024
Copy link
Member

@axelavargas axelavargas left a comment

Choose a reason for hiding this comment

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

Hey @kaydelaney, I think this approach works better for our use case 🌟 . I gave it a local test with a dummy scenes app and the Grafana core repository.

Dummy scenes app

NewSearchAndPanelPerRow.mp4

Grafana core

CoreGrafanaDashboardTest.mp4

UX considerations:

Including "search by title" and "resize per row" in the layout canvas might not give the best user experience. However, we're planning to hide this feature behind a toggle in core Grafana, so it'll only be available to a specific customer and won't affect the wider user base much. Also, not many Grafana applications use the SceneGridLayout.

What do you think, @torkelo? Do you have strong opinions on this or a better idea for solving the problem? 🙃

Comment on lines 377 to 381
const filteredChildren = this.state.children.filter(
(v) =>
typeof (v.state as any).body?.state?.title === 'string' &&
(v.state as any).body.state.title.toLowerCase().includes(panelFilterInterpolated)
);
Copy link
Member

Choose a reason for hiding this comment

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

We can also interpolate the title , we could match variables that are in the title, something like:

Suggested change
const filteredChildren = this.state.children.filter(
(v) =>
typeof (v.state as any).body?.state?.title === 'string' &&
(v.state as any).body.state.title.toLowerCase().includes(panelFilterInterpolated)
);
const filteredChildren = this.state.children.filter(
(v) => {
const panelTitleInterpolated = interpolate(v, (v.state as any).body?.state?.title).toLowerCase();
return typeof (v.state as any).body?.state?.title === 'string' &&
(panelTitleInterpolated.includes(panelFilterInterpolated));
}
);

@kaydelaney kaydelaney marked this pull request as ready for review September 24, 2024 10:36
@kaydelaney
Copy link
Contributor Author

Closing in favor of grafana/grafana#93670

@kaydelaney kaydelaney closed this Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged release Create a release when this pr is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants