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

PluginExtensions: Add category to link extensions #71074

Merged
merged 9 commits into from
Jul 24, 2023
Merged

Conversation

mckn
Copy link
Contributor

@mckn mckn commented Jul 5, 2023

What is this feature?
To prevent extensions for repeating their origin as part of the title we have introduced the possibility to define a category that the extensions will be group below.

Before:
Screenshot 2023-07-21 at 14 05 43

After:
Screenshot 2023-07-21 at 13 32 31

Why do we need this feature?
It will improve the user experience and make it easier for the end user to find what they are looking for.

Who is this feature for?
plugin developers, end users

Which issue(s) does this PR fix?:
Fixes #69291

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.

@mckn mckn self-assigned this Jul 5, 2023
@mckn mckn added this to the 10.1.x milestone Jul 5, 2023
@mckn mckn added add to changelog no-backport Skip backport of PR labels Jul 5, 2023
@mckn mckn marked this pull request as ready for review July 21, 2023 12:13
@mckn mckn requested review from a team as code owners July 21, 2023 12:13
@mckn mckn requested review from academo, dprokop, kaydelaney, tskarhed, JoaoSilvaGrafana and jackw and removed request for a team July 21, 2023 12:13
@mckn
Copy link
Contributor Author

mckn commented Jul 21, 2023

@sd2k maybe something we can use in ML?

@sd2k
Copy link
Contributor

sd2k commented Jul 24, 2023

Yep this looks useful for ML! Should be backwards compatible too which is ace 🎉

Copy link
Contributor

@jackw jackw left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@sd2k
Copy link
Contributor

sd2k commented Jul 24, 2023

Should be backwards compatible too which is ace 🎉

Hmm I'm not actually sure this is true (in some senses) - if I add the category field to my extension link then they aren't shown in Grafana 10.0 😞 I guess there's something in Grafana's getExtensionLinks which checks for unknown properties? Not much that can be done about that now though, I'll just wait for this to be released before adding the category.

Edit:

FYI the error I see in the logs is:

[Plugin Extensions] Invalid extension "Create forecast". Trying to override not-allowed properties: category

@mckn
Copy link
Contributor Author

mckn commented Jul 24, 2023

Should be backwards compatible too which is ace 🎉

Hmm I'm not actually sure this is true (in some senses) - if I add the category field to my extension link then they aren't shown in Grafana 10.0 😞 I guess there's something in Grafana's getExtensionLinks which checks for unknown properties? Not much that can be done about that now though, I'll just wait for this to be released before adding the category.

Edit:

FYI the error I see in the logs is:

[Plugin Extensions] Invalid extension "Create forecast". Trying to override not-allowed properties: category

Is it a warning or an error? We probably print something like this for the developers but I will make sure it is an error and that it only is printed if you are running Grafana in development mode.

@mckn mckn merged commit 57a54fc into main Jul 24, 2023
17 checks passed
@mckn mckn deleted the mckn/ui-extensions-group branch July 24, 2023 11:09
@sd2k
Copy link
Contributor

sd2k commented Jul 24, 2023

It's logged to the console as a warning 👍

@mckn
Copy link
Contributor Author

mckn commented Jul 24, 2023

It's logged to the console as a warning 👍

But it will still show the extension, right? Or will it disable the extension after that warning. If that is the case we probably have a bug.

@sd2k
Copy link
Contributor

sd2k commented Jul 24, 2023

It disables the extension 😞 (just to make sure, I'm talking about when trying to use extensions with a category with Grafana versions older than this PR)

@mckn
Copy link
Contributor Author

mckn commented Jul 24, 2023

It disables the extension 😞 (just to make sure, I'm talking about when trying to use extensions with a category with Grafana versions older than this PR)

Oh, then I need to fix that for 10.1.0 and probably backport it to next 10.0.x release. We should print a warning, ignore the property but not disable the extension in this scenario.

@mckn
Copy link
Contributor Author

mckn commented Jul 24, 2023

Thanks for reporting this! 🙏🏻 I will fix this during the day tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Plugins: Introduce a category header for extensions in panel menu
5 participants