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

Dashboard: Add new visualization/row/library panel/pasted panel is now a dropdown menu #65361

Merged
merged 24 commits into from Mar 30, 2023

Conversation

polibb
Copy link
Contributor

@polibb polibb commented Mar 27, 2023

What is this feature?

Implement dropdown to add a new viz/library panel/row/pasted panel in the dashboard. Implemented in place of the current button in the top bar.
This PR:

  • creates utils methods for creating new panel, new row, importing from library, and pasting a panel from clipboard
  • creates a dropdown menu to be shown when clicking on Add icon in dashboard's nav bar
  • removes always setting the panel-add icon to size xl and sets it to xl everywhere it was used until now
  • removes automatic creation of new panel of type "add-panel" when calling getNewDashboardModelData when initialising a dashboard

Why do we need this feature?

Because it is better UX.

Who is this feature for?

Everyone

Which issue(s) does this PR fix?:

Fixes #64592

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.
  • There are no known compatibility issues with older supported versions of Grafana, or plugins.
  • It passes the Hosted Grafana feature readiness review for observability, scalability, performance, and security.

@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/pr-codeql-analysis-javascript.yml:analyze. As part of the setup process, we have scanned this repository and found 29 existing alerts. Please check the repository Security tab to see all alerts.

@polibb polibb changed the title Polibb/dashboard add btn Dashboard: Add new visualization/row/library panel/pasted panel is now a dropdown menu Mar 28, 2023
@polibb polibb marked this pull request as ready for review March 28, 2023 15:17
@polibb polibb requested review from a team as code owners March 28, 2023 15:17
@polibb polibb requested review from axelavargas, kaydelaney, tskarhed and yaelleC and removed request for a team March 28, 2023 15:17
@polibb polibb requested review from tskarhed and yaelleC and removed request for tskarhed and yaelleC March 28, 2023 15:20
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.

Great job @polibb 🥳 , the code it's easy to read and well organized, I gave it a try locally and it works as expected, both with and without the feature flag.

P.S., I left a small nit, and I think we need some unit tests for the new components (but if time is scarce, we could always add the tests in a different PR).

offset={[0, 6]}
onVisibleChange={setAddMenuOpen}
>
<Button
Copy link
Member

Choose a reason for hiding this comment

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

Did you figure out why ToolbarButton cannot be used here? Would love to understand that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no 🥸

dashboard.addPanel(newPanel);
};

type PanelPluginInfo = { defaults: { gridPos: { w: number; h: number }; title: string } };
Copy link
Member

Choose a reason for hiding this comment

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

A very similar type is defined in AddPanelWidget. Can we re-use that one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the id of PanelPluginInfo in AddPanelWidget is of type number, whereas we are looking for a panel in panels here which are of type PanelPluginMeta which has the id of type string. (I suppose that's why const pluginInfo is set of type any, but that's not possible anymore - betterer is failing if I recreate the same method.)
The id is used as a holder for the panel's type in this search for the copied panel and then later for the creation of the pasted panel, so it suits it to be a string 😄.

I think we'll delete this AddPanelWidget alltogether when the feature flag emptyDashboardPage is removed?

Tbh, as a whole I was confused by this method - it's creating an array, but only ever fills it with 1 value; I'm not sure if pluginCopy.sort = -1; is necessary and why. The main reason I think any was used is because the defaults key is set onto the panelCopy (pluginCopy.defaults = copiedPanel;), but overall I think we can either return a tuple or this kind of an object of combined types which are just the panel type and a type that cares about defaults.

<Menu>
<Menu.Item
key="add-visualisation"
label="Visualization"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking the PR, but shouldn’t we localise these?

@polibb polibb merged commit b9fb235 into main Mar 30, 2023
15 checks passed
@polibb polibb deleted the polibb/dashboard-add-btn branch March 30, 2023 10:50
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.

Dashboard: Add new panel button is a dropdown with options
6 participants