-
Notifications
You must be signed in to change notification settings - Fork 32
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
PluginE2E: Added (app) plugin config fixture #795
Conversation
Hello! 👋 This repository uses Auto for releasing packages using PR labels. ✨ This PR can be merged but will not trigger a new release. To trigger a new release add the |
packages/plugin-e2e/src/fixtures/commands/createAppConfigPage.ts
Outdated
Show resolved
Hide resolved
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.
This looks great! Just some open questions to you in the comments.
packages/plugin-e2e/tests/as-admin-user/app/app-config/appConfig.spec.ts
Outdated
Show resolved
Hide resolved
packages/plugin-e2e/src/fixtures/commands/createAppConfigPage.ts
Outdated
Show resolved
Hide resolved
const url = this.ctx.selectors.pages.Plugin.url(this.args.pluginId); | ||
return super.navigate(url, options); | ||
} | ||
} |
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.
I know the Save
button is not part of the app API. However, it's there when you scaffold a new app plugin and we have it in the examples repo. So maybe we should also expose a Save
method here?
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.
I was thinking about this back and forth and it feels a bit weird to expose a save method if the plugin choose to name that button something else.
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.
Yeah it's a bit weird. 👍 If we decide to add a onOptionsChang
method along with a save button to the app config api, we can add a save button here too.
packages/plugin-e2e/tests/as-admin-user/app/app-config/appConfig.spec.ts
Outdated
Show resolved
Hide resolved
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.
Nice!
🚀 PR was released in |
Description
Adding fixture to make it easier to test app configs pages. This is a thin abstraction and I need to verify this against older versions of Grafana prior to merging it.
Todo:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
📦 Published PR as canary version:
Canary Versions
✨ Test out this PR locally via:
npm install @grafana/plugin-e2e@0.20.0-canary.795.f5d5401.0 # or yarn add @grafana/plugin-e2e@0.20.0-canary.795.f5d5401.0