-
Notifications
You must be signed in to change notification settings - Fork 39
Suggestions: Add All Visualizations click for 12.4 and greater #2399
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a defensive click on the "All Visualizations" tab in the visualization picker for Grafana 12.4.0 and later to ensure tests work correctly with the new Suggestions feature introduced in that version.
Changes:
- Added version-gated logic to click "All Visualizations" tab before selecting a visualization type in panel edit page
- Ensures compatibility with the new Suggestions feature rolled out in Grafana 12.4.0
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
not sure if this is a patch or minor change. |
Playwright test results
Troubleshooting404 when clicking on
|
sunker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for considering plugin-e2e and raising this pr @fastfrwrd!
Code looks good to me! Would you be able to add a simple e2e tests that covers the new scenario? You can add it here.
To test it locally:
# start Grafana (with new suggestions feature)
GRAFANA_IMAGE=grafana-dev GRAFANA_VERSION=12.4.0-21043112277 npm run server --w @grafana/plugin-e2e
# or without it
GRAFANA_VERSION=12.3.0 npm run server --w @grafana/plugin-e2e
# run e2e tests
npm run playwright:test --w @grafana/plugin-e2e| // when suggestions got updated in 12.4.0, it became necessary to ensure the "All Visualizations" tab is selected, | ||
| // and also we need to check whether we're already rendering the viz picker or not since creating a new panel shows | ||
| // the viz picker by default when the panel editor is opened | ||
| if (semver.gte(this.ctx.grafanaVersion, '12.4.0')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is the case, but If the new suggestions feature is behind a feature flag, you may use the semver check in combination with a feature flag check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much of "new suggestions" is behind a flag, but one thing that isn't behind a feature flag is that we now track whatever tab on the viz picker you've clicked on ("All visualizations" or "Suggestions") and persist that in sessionStorage as a quasi "preference" for the next time we render that picker. It's probably safer to just always check whether we're on "All visualizations" before proceeding just in case a previous test touched Suggestions at all now.
What this PR does / why we need it:
With the new Suggestions rolling out, it's not a guarantee that the All Visualizations tab is open when you toggle the viz picker (it often will be suggestions showing instead). This adds a step where we ensure All Visualizations is selected before we click the viz type. It's also not a guarantee anymore that the Panel Options is rendered when you first visit a panel - we are showing the viz picker (with suggestions selected) by default in the new flag, so we also need logic to help make sure that we don't toggle the viz picker if it's already showing.
Special notes for your reviewer:
I added the version check out of an abundance of caution, but this should work several more versions back without it.