-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Move panel placement register to dashboard start contract #182571
Move panel placement register to dashboard start contract #182571
Conversation
Also renames legacy placement method
/ci |
} | ||
return { width: 12, height: 8 }; | ||
} | ||
); |
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 moved this to the start lifecycle since the method is provided by DashboardStart
.
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
Pinging @elastic/kibana-presentation (Team:Presentation) |
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.
kibana-presentation changes LGTM
code review only
/ci |
@@ -159,6 +149,16 @@ export class SloPlugin | |||
const kibanaVersion = this.initContext.env.packageInfo.version; | |||
const { ruleTypeRegistry, actionTypeRegistry } = pluginsStart.triggersActionsUi; | |||
|
|||
pluginsStart.dashboard.registerDashboardPanelPlacementSetting( |
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.
@nickpeihl The SLO feature is only for users with platinum license and above. Can you check for proper license here? Let me know if you want me to do the change and push directly to your branch
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.
@nickpeihl I was now checking a draft PR by @ThomThomson and I noticed he didn't move the registration to the start
method. He kept things in the setup method.
In his case he changed to pluginsSetup.embeddable.registerReactEmbeddableFactory
. So maybe you want to try doing something similar pluginsSetup.dashboard.registerDashboardPanelPlacementSetting
, instead of moving the whole thing in the start method and having to do the platinum 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.
Oops, you're right. I was misunderstanding the lifecycle. DashboardStart
is available in the plugins' Setup
lifecycle via the core.getStartServices
method. Fixed in 79d8d9c.
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.
Ok looks good! Let me quickly try it out locally, and then I'll happily approve
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.
@nickpeihl I tested it and I verify that all works well!
…hboard-start' into panel-placement-dashboard-start
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Canvas Sharable Runtime
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
Thank you for doing this! LGTM!
Part of #182585
Summary
Puts the
registerDashboardPanelPlacementSetting
function on the Dashboard start contract.Rather than importing the module directly, consumers will need to include
dashboard
in their plugin's start dependencies.