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

[ML] Decouple data_visualizer from MapEmbeddable #181928

Merged
merged 13 commits into from
May 1, 2024
Merged

[ML] Decouple data_visualizer from MapEmbeddable #181928

merged 13 commits into from
May 1, 2024

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Apr 26, 2024 •
edited
Loading

Part of #182020

test instructions

  1. install web logs sample data
  2. Open discover
  3. In table, click "Field statistics"
  4. Expand geo.dest row. Verify choropleth map is displayed.
  5. Expand geo.coordinates row. Verify vector map is displayed.

@nreese
Copy link
Contributor Author

nreese commented Apr 29, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Apr 29, 2024

/ci

@nreese nreese mentioned this pull request Apr 29, 2024
Closed
5 tasks
@nreese
Copy link
Contributor Author

nreese commented Apr 29, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Apr 29, 2024

/ci

@nreese nreese marked this pull request as ready for review April 29, 2024 22:05
@nreese nreese requested review from a team as code owners April 29, 2024 22:05
@nreese nreese added :ml Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas project:embeddableRebuild v8.15.0 labels Apr 29, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@elasticmachine
Copy link
Contributor

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

@nreese
Copy link
Contributor Author

nreese commented Apr 29, 2024

@elasticmachine merge upstream

@nreese nreese added the release_note:skip Skip the PR/issue when compiling release notes label Apr 29, 2024
@qn895
Copy link
Member

qn895 commented Apr 30, 2024

Tested and LGTM 🎉

@peteharverson peteharverson changed the title decouple data_visualizer from MapEmbeddable [ML] Decouple data_visualizer from MapEmbeddable Apr 30, 2024
nickpeihl
nickpeihl approved these changes May 1, 2024
View reviewed changes
Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

lgtm! nice cleanup!

code review and tested Map embeddable in Discover Field Statistics.

x-pack/plugins/maps/public/legacy_visualizations/tile_map/tile_map_renderer.tsx
const getLazyComponent = () => {
return lazy(() => import('./tile_map_visualization'));
};
const Component = dynamic(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Oh neat. So using dynamic avoids needing to specify // eslint-disable-next-line import/no-default-export?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not quite. returning { default } prevents needing a component to export as default

@nreese
Copy link
Contributor Author

nreese commented May 1, 2024

@elasticmachine merge upstream

andreadelrio
andreadelrio approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataVisualizer 651 644 -7
maps 1178 1182 +4
total -3

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
maps 274 276 +2

Async chunks

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

id before after diff
dataVisualizer 658.5KB 656.2KB -2.3KB
maps 3.0MB 3.0MB +6.5KB
total +4.1KB

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
maps 31 32 +1

Page load bundle

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

id before after diff
dataVisualizer 24.0KB 23.9KB -134.0B
maps 50.7KB 51.4KB +691.0B
total +557.0B
Unknown metric groups

API count

id before after diff
maps 275 281 +6

async chunk count

id before after diff
maps 28 30 +2

ESLint disabled line counts

id before after diff
dataVisualizer 72 67 -5
maps 52 54 +2
total -3

Total ESLint disabled count

id before after diff
dataVisualizer 73 68 -5
maps 79 81 +2
total -3

History

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

@nreese nreese merged commit 1e8d51a into elastic:main May 1, 2024
19 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 1, 2024
TinLe added a commit to TinLe/kibana that referenced this pull request May 1, 2024
@TinLe
Merge branch 'master' into dependabot/npm_and_yarn/nodemailer-6.9.9
56f9aa4 
* master: (1654 commits)
  Bump ejs from 3.1.9 to 3.1.10
  Don't render exceptions flyout if data is loading (elastic#181588)
  Enable value list modal (elastic#181593)
  skip flaky suite (elastic#181777)
  skip failing test suite (elastic#182263)
  [Mappings Editor] Disable _source field in serverless (elastic#181712)
  [data.search] Fix unhandled promise rejections (elastic#181785)
  [Fleet] Fix logic for detecting first time Elastic Agent users (elastic#182214)
  [ML] Decouple data_visualizer from MapEmbeddable (elastic#181928)
  [ES|QL] Sorting accepts expressions (elastic#181916)
  [ML] Single Metric Viewer: ensures chart displays correctly when opening from a job annotation (elastic#182176)
  Adding optional Description field to Roles APIs (elastic#182039)
  Upgrade Markdown-it to 14.1.0 (elastic#182244)
  Bump xml-crypto from 5.0.0 to 6.0.0
  [DOCS] Fix docs and screenshots for rule creation changes (elastic#181925)
  Update dependency elastic-apm-node to ^4.5.3 (main) (elastic#182236)
  [Obs AI Assistant] register alert details context in observability plugin (elastic#181501)
  Add `@typescript-eslint/no-floating-promises` (elastic#181456)
  [Playground] Propagate Error message into FE (elastic#182201)
  [ES|QL] Rename the setting to a more generic one and move to the general section (elastic#182074)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qn895 qn895 qn895 approved these changes

@andreadelrio andreadelrio andreadelrio approved these changes

@nickpeihl nickpeihl nickpeihl approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting :ml project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.15.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nreese @elasticmachine @qn895 @kibana-ci @nickpeihl @andreadelrio @kibanamachine