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

[Embeddables as Building Blocks] Controls API #140739

Merged
merged 11 commits into from
Oct 19, 2022

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Sep 14, 2022

Summary

This PR introduces a strongly typed React Component, control_group_renderer which returns a reference to the full Control Group Embeddable, so that the consuming application can use it as an API. This PR also contains a few new API additions for the Control group. This pattern should be used as an example moving forward for Embeddables as Building Blocks, and is necessary for the Portable Dashboards initiative.

@ThomThomson ThomThomson marked this pull request as ready for review October 14, 2022 15:12
@ThomThomson ThomThomson requested a review from a team as a code owner October 14, 2022 15:12
@ThomThomson ThomThomson added Feature:Input Control Input controls visualization release_note:skip Skip the PR/issue when compiling release notes Project:Controls Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Oct 14, 2022
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

Code review LGTM, left a few comments + one clarifying question 👍

However, I found a bug when testing locally:

  1. Load one of the sample dashboards from scratch
  2. Make a selection
  3. Unselect that selection
  4. Notice that the unsaved changes badge doesn't disappear even though it should
Screen.Recording.2022-10-14.at.4.48.28.PM.mov

/**
* 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.

This is a cool hook 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I enjoy these more explicitly named hooks from this library.

If you noticed, I imported it individually up top, because something in that library was failing our eslint in some manner. So if we use this function, we have to be careful with it.

Comment on lines +43 to +45
return i18n.translate('controls.controlGroup.title', {
defaultMessage: 'Control group',
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figure you got what this change did, but I'll write it down here in case anyone else is looking:

Removing the reference to the strings file here makes it so that the entire strings file doesn't end up in the page load bundle.

onEmbeddableLoad: (controlGroupContainer: ControlGroupContainer) => void;
}

export const ControlGroupRenderer = ({ input, onEmbeddableLoad }: ControlGroupRendererProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear, this ControlGroupRenderer isn't actually used anywhere yet, correct? You are simply introducing it to use it in a follow-up PR related to portable dashboards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely correct.

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

Approving because the bug I found is actually in main and not related to this PR 👍

@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
controls 206 215 +9

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 205 220 +15

Async chunks

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

id before after diff
controls 471.3KB 491.5KB +20.2KB

Page load bundle

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

id before after diff
controls 26.4KB 18.4KB -8.1KB
Unknown metric groups

API count

id before after diff
controls 213 229 +16

async chunk count

id before after diff
controls 7 8 +1

ESLint disabled line counts

id before after diff
controls 1 2 +1

Total ESLint disabled count

id before after diff
controls 1 2 +1

History

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

@ThomThomson ThomThomson merged commit c8a0376 into elastic:main Oct 19, 2022
@kibanamachine kibanamachine added v8.6.0 backport:skip This commit does not require backporting labels Oct 19, 2022
nreese added a commit that referenced this pull request Nov 15, 2022
…45184)

#140739 caused a regression with
timeslider control where `ControlGroupContainer` was not not firing
`updateOutput` on `output.timeslice` changes.

This PR resolves the regression and adds functional tests to ensure
interactions with timeslider properly flow to dashboard saved search
panel

Co-authored-by: kibanamachine <[email protected]>
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 Feature:Input Control Input controls visualization Project:Controls release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants