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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Controls] [Portable Dashboards] Add control group renderer example plugin #146189

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented Nov 23, 2022

Closes #145429

Summary

This PR expands on the control group building block by

  1. Replacing the old input declarative API and replacing it with a getCreationOptions callback
  2. Exposing the redux embeddable tools to the consumer

As part of this, I created an example plugin to demonstrate some of the new functionality 馃憤

Nov-23-2022 10-27-08

Also, to avoid some code duplication, I had to move some code to the generic control_group_helpers.tsx so that both the controls plugin and the new example plugin could use it.

Checklist

For maintainers

@Heenawter Heenawter added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. backport:skip This commit does not require backporting Project:Controls Project:Portable Dashboard Related to the Portable Dashboards initiative v8.7.0 labels Nov 23, 2022
@Heenawter Heenawter self-assigned this Nov 23, 2022
@Heenawter Heenawter force-pushed the enhance-control-group-building-block_2022-11-18 branch 4 times, most recently from 1f1d8dc to a3f800e Compare November 23, 2022 17:32
@Heenawter Heenawter force-pushed the enhance-control-group-building-block_2022-11-18 branch from a3f800e to bbb2342 Compare November 23, 2022 17:39
@Heenawter Heenawter marked this pull request as ready for review November 23, 2022 17:50
@Heenawter Heenawter requested a review from a team as a code owner November 23, 2022 17:50
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@Heenawter Heenawter force-pushed the enhance-control-group-building-block_2022-11-18 branch from 0c7a4a3 to bf5bfe4 Compare November 23, 2022 18:00
@kibana-ci
Copy link
Collaborator

馃挌 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
controls 157 158 +1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
controls 228 231 +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
controls 475.4KB 451.5KB -23.9KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
controls 7 8 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
controls 21.8KB 30.6KB +8.7KB
Unknown metric groups

API count

id before after diff
controls 237 240 +3

async chunk count

id before after diff
controls 8 6 -2

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 109 115 +6
securitySolution 443 449 +6
total +20

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 68 74 +6
osquery 110 117 +7
securitySolution 520 526 +6
total +21

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @Heenawter

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

LGTM! Code only review. The only comments I added are related to forward-looking future things and ideas, but all the code in this PR is exactly what I was hoping for. Nice work!

{ data }: ControlsExampleStartDeps,
{ element }: AppMountParameters
) => {
const dataViews = await data.dataViews.find('kibana_sample_data_ecommerce');
Copy link
Contributor

Choose a reason for hiding this comment

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

Not required for this PR - as this one is mostly about upgrading the API, but food for thought - maybe for all of our examples, we should align behind the sample web logs data set, as it has more relevance to solution use cases.

Additionally, maybe we should show some sort of warning asking developers who want to use the examples to install this data set if it doesn't exist.

interface Props {
dataView: DataView;
}
const ControlGroupRenderer = withSuspense(LazyControlGroupRenderer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, not for right now, but I've been thinking about our usages of withSuspense, and how we actually export the LazyControlGroupRenderer. I'm thinking that maybe we should write our own version of withSuspense that only works with the Control Group (with a better / centered loading indicator), and export that instead so that implementors don't need to also use presentation_util.

onEmbeddableLoad: (controlGroupContainer: ControlGroupContainer) => void;
getCreationOptions: (
builder: typeof ControlGroupInputBuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

馃挴

const id = useMemo(() => uuid.v4(), []);

/**
* Use Lifecycles to load initial control group container
*/
useLifecycles(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that while I was working on Portable Dashboard I ran into some problem with the useLifecycles hook which made me replace it with a general useEffect. Not sure exactly what the problem was anymore because it was a while ago, but I'd make sure that it gets called when you expect it to, and that the cleanup function works as / when expected.

import { getCompatibleControlType, getNextPanelOrder } from './embeddable/control_group_helpers';
import { controlGroupReducers } from './state/control_group_reducers';

const ControlGroupInputBuilder = {
Copy link
Contributor

Choose a reason for hiding this comment

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

The next step of this process will be to use an input builder like this one in the inline editor to serve the Control Creation use case. This is because the control editor in that case simply returns a partial input object for the Control type you're creating, so it would be cool to eventually use the same APIs for partial input creation inside or outside of the Controls plugin.

const ControlGroupInputBuilder = {
addDataControlFromField: async (
initialInput: Partial<ControlGroupInput>,
newPanelInput: {
Copy link
Contributor

Choose a reason for hiding this comment

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

@nreese, I'm appreciating your suggestion to expose API methods here for individual control types, but I think we should also keep this generic one for simplicity.

In the individual control type builders, we would be able to take selections, in case a consumer wanted to build an Options List control for instance with some options pre-selected.

@Heenawter Heenawter merged commit bb91749 into elastic:main Nov 24, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 28, 2022
* main: (30 commits)
  [Cloud Posture] test latest findings table sort (elastic#144668)
  [api-docs] 2022-11-28 Daily api_docs build (elastic#146359)
  [api-docs] 2022-11-27 Daily api_docs build (elastic#146353)
  [api-docs] 2022-11-26 Daily api_docs build (elastic#146350)
  [DataViews] Fix form validation UX when the same data view name already exists (elastic#146126)
  [Discover] Prevent agg based visualizations of Discover saved objects with adhoc data views (elastic#145583)
  [Health Gateway] Update response aggregation (elastic#145761)
  [api-docs] 2022-11-25 Daily api_docs build (elastic#146341)
  [Metric threshold rule] Adds new context variable for group by keys (elastic#145654)
  [Controls] [Portable Dashboards] Add control group renderer example plugin (elastic#146189)
  Refactor Observability Overview Page (elastic#146182)
  Send complete test data to xMatters, so it can create an alert (elastic#145431)
  [Dashboard] [Controls] Allow options list suggestions to be sorted (elastic#144867)
  Add open API specification for list connector types (elastic#145951)
  skip flaky suite (elastic#146086)
  [ML] Removing duplicate tooltip text (elastic#146308)
  Refactor Rules Page (elastic#146193)
  [DOCS] Alert limit for cases (elastic#145950)
  Extend session index fields mapping with a session creation timestamp. (elastic#145997)
  [Files] Move <Image /> component to `@kbn/shared-ux` package (elastic#145995)
  ...
@Heenawter Heenawter deleted the enhance-control-group-building-block_2022-11-18 branch December 30, 2022 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Project:Controls Project:Portable Dashboard Related to the Portable Dashboards initiative release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Portable Dashboards][Controls] Enhance Control Group Building Block
4 participants