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.

Sign up for GitHub

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

Jump to bottom

[Lens] Decouple Open in Discover Action and Drilldown from Embeddable framework #177888

Merged
merged 13 commits into from
Mar 19, 2024
Merged

[Lens] Decouple Open in Discover Action and Drilldown from Embeddable framework #177888

merged 13 commits into from
Mar 19, 2024

Conversation

nickpeihl
Copy link
Member

@nickpeihl nickpeihl commented Mar 1, 2024 •
edited
Loading

Summary

Decouples the "Explore data in Discover" panel action and the "Open in Discover" drilldown from the legacy embeddable framework.

part of #175138

@nickpeihl
Copy link
Member Author

/ci

@nickpeihl nickpeihl mentioned this pull request Mar 4, 2024
Closed
@nickpeihl
Copy link
Member Author

/ci

@nickpeihl
Copy link
Member Author

/ci

@nreese nreese self-requested a review March 5, 2024 12:58
nreese
nreese reviewed Mar 5, 2024
View reviewed changes
x-pack/plugins/lens/public/trigger_actions/types.ts Outdated
import { apiIsOfType } from '@kbn/presentation-publishing';
import { HasLensConfig, ViewUnderlyingDataArgs } from '../embeddable';

export type OpenInDiscoverActionApi = HasLensConfig & {
Copy link
Contributor

@nreese nreese Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

New interface definitions are defined for embeddables, not actions. Action APIs just define a collection of existing interfaces but does not add new interface definitions.

I think there are 2 ways forward

  1. move canViewUnderlyingData and getViewUnderlyingDataArgs into HasLensConfig API since they are properties of a lens API
  2. Create a interface file as a sibling to has_lens_config, something like PublishesUnderlyingData, and move this API and type guard into that interface.

I think I like option 1 the best since this part of lens API. @ThomThomson thoughts?

Copy link
Member Author

@nickpeihl nickpeihl Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. I've seen the LensApi changes you are making in https://github.com/elastic/kibana/pull/176869/files#diff-cf9423bd506b04c1decd8629e55629bdd19b74ad59f869f3abe994c1d364b2bc and Option 1 makes sense.

Option 2 would also work, but it seems like an unnecessary abstraction.

@nickpeihl
Copy link
Member Author

/ci

@nickpeihl nickpeihl added release_note:skip Skip the PR/issue when compiling release notes v8.14.0 labels Mar 11, 2024
@nickpeihl nickpeihl marked this pull request as ready for review March 11, 2024 17:25
@nickpeihl nickpeihl requested a review from a team as a code owner March 11, 2024 17:25
@nickpeihl nickpeihl requested a review from nreese March 11, 2024 17:25
@nickpeihl nickpeihl added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Mar 11, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@elasticmachine
Copy link
Contributor

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

nreese
nreese approved these changes Mar 11, 2024
View reviewed changes
Copy link
Contributor

@nreese nreese 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 review only

@nickpeihl nickpeihl self-assigned this Mar 12, 2024
@nickpeihl
Copy link
Member Author

/ci

@nickpeihl
Copy link
Member Author

/ci

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
lens 1.4MB 1.4MB -4.0B

Page load bundle

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

id before after diff
lens 45.8KB 45.9KB +59.0B

History

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

cc @nickpeihl

mbondyra
mbondyra approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Tested on Chrome, the changes look good to me 👌🏼

@nickpeihl nickpeihl merged commit 9dc14d6 into elastic:main Mar 19, 2024
17 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbondyra mbondyra mbondyra approved these changes

@nreese nreese nreese approved these changes

Assignees

@nickpeihl nickpeihl

Labels
backport:skip This commit does not require backporting project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nickpeihl @elasticmachine @kibana-ci @nreese @mbondyra @kibanamachine