Skip to content

[embeddable refactor] decouple MapEmbeddable from Security Solution plugin#182737

Merged
nreese merged 12 commits intoelastic:mainfrom
nreese:decouple_map_embeddable_from_security_solution
May 13, 2024
Merged

[embeddable refactor] decouple MapEmbeddable from Security Solution plugin#182737
nreese merged 12 commits intoelastic:mainfrom
nreese:decouple_map_embeddable_from_security_solution

Conversation

@nreese
Copy link
Copy Markdown
Contributor

@nreese nreese commented May 6, 2024

Part of #182020

@elastic/kibana-presentation is refactoring the Embeddable framework. Part of this effort is decoupling plugins from the legacy Embeddable framework. The Maps plugin provides a react component from the plugins start contract that can be used instead of natively using MapEmbeddable.

This PR swaps out the native MapEmbeddable implementation with the react component exposed from the Maps plugin.

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented May 6, 2024

/ci

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented May 6, 2024

/ci

@nreese nreese marked this pull request as ready for review May 6, 2024 22:13
@nreese nreese requested review from a team as code owners May 6, 2024 22:13
@nreese nreese added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// label May 6, 2024
Comment on lines +179 to +210
const content = !storageValue ? null : (
<Embeddable>
<InPortal node={portalNode}>
<MapToolTip />
</InPortal>

<EmbeddableMap maintainRatio={!isIndexError}>
{isIndexError ? (
<IndexPatternsMissingPrompt data-test-subj="missing-prompt" />
) : (
<services.maps.Map
// eslint-disable-next-line react/display-name
getTooltipRenderer={() => (tooltipProps: RenderTooltipContentParams) =>
<OutPortal node={portalNode} {...tooltipProps} />}
mapCenter={{ lon: -1.05469, lat: 15.96133, zoom: 1 }}
layerList={layerList}
filters={appliedFilters}
query={query}
onApiAvailable={(api: MapApi) => {
// Wire up to app refresh action
setQuery({
id: 'embeddedMap', // Scope to page type if using map elsewhere
inspect: null,
loading: false,
refetch: () => api.reload(),
});
}}
/>
)}
</EmbeddableMap>
</Embeddable>
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning this up for Security Solution @nreese ,

I've tested locally and found there's some styling issue:

Screenshot 2024-05-08 at 11 20 40

I have a suggested solution to fix this:

  1. Please rename:
    EmbeddableMap to EmbeddableMapRatioHolder

  2. Please add:

const EmbeddableMapWrapper = styled.div`
  position: relative;
`;

const EmbeddableMap = styled.div`
  height: 100%;
  width: 100%;
  position: absolute;
  top: 0;
`;
  1. Please update the content:
Suggested change
const content = !storageValue ? null : (
<Embeddable>
<InPortal node={portalNode}>
<MapToolTip />
</InPortal>
<EmbeddableMap maintainRatio={!isIndexError}>
{isIndexError ? (
<IndexPatternsMissingPrompt data-test-subj="missing-prompt" />
) : (
<services.maps.Map
// eslint-disable-next-line react/display-name
getTooltipRenderer={() => (tooltipProps: RenderTooltipContentParams) =>
<OutPortal node={portalNode} {...tooltipProps} />}
mapCenter={{ lon: -1.05469, lat: 15.96133, zoom: 1 }}
layerList={layerList}
filters={appliedFilters}
query={query}
onApiAvailable={(api: MapApi) => {
// Wire up to app refresh action
setQuery({
id: 'embeddedMap', // Scope to page type if using map elsewhere
inspect: null,
loading: false,
refetch: () => api.reload(),
});
}}
/>
)}
</EmbeddableMap>
</Embeddable>
);
const content = !storageValue ? null : (
<Embeddable>
<InPortal node={portalNode}>
<MapToolTip />
</InPortal>
<EmbeddableMapWrapper>
<EmbeddableMapRatioHolder maintainRatio={!isIndexError} />
{isIndexError ? (
<IndexPatternsMissingPrompt data-test-subj="missing-prompt" />
) : (
<EmbeddableMap>
<services.maps.Map
// eslint-disable-next-line react/display-name
getTooltipRenderer={() => (tooltipProps: RenderTooltipContentParams) =>
<OutPortal node={portalNode} {...tooltipProps} />}
mapCenter={{ lon: -1.05469, lat: 15.96133, zoom: 1 }}
layerList={layerList}
filters={appliedFilters}
query={query}
onApiAvailable={(api: MapApi) => {
// Wire up to app refresh action
setQuery({
id: 'embeddedMap', // Scope to page type if using map elsewhere
inspect: null,
loading: false,
refetch: () => api.reload(),
});
}}
/>
</EmbeddableMap>
)}
</EmbeddableMapWrapper>
</Embeddable>
);

expected:

Screenshot 2024-05-08 at 10 40 47

@angorayc
Copy link
Copy Markdown
Contributor

angorayc commented May 8, 2024

Some other differences I found:

  1. There are two base maps in the layer:
Screenshot 2024-05-08 at 12 01 40
  1. The new map doesn't seem to refetch when the refetch button is clicked:
    Before:
Screen.Recording.2024-05-08.at.12.02.56.mov

In this PR:

Screen.Recording.2024-05-08.at.12.03.16.mov

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented May 8, 2024

@elasticmachine merge upstream

@nickpeihl nickpeihl self-requested a review May 8, 2024 20:02
@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented May 9, 2024

I've tested locally and found there's some styling issue:

Resolved with 590602b

There are two base maps in the layer:

Resolved with b559286

The new map doesn't seem to refetch when the refetch button is clicked:

Data is getting refetched. To view this, open up browser debugging tools and open network tab. Set network throttling to "Slow 3G". Loading icons will appear for the layers as they reload

Screenshot 2024-05-09 at 9 19 25 AM

@nreese nreese requested a review from angorayc May 9, 2024 15:19
@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented May 9, 2024

@elasticmachine merge upstream

Copy link
Copy Markdown
Contributor

@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! great to simplify the usage with the Map component!

code review and tested Network Map in Security with sample data 🎩

Copy link
Copy Markdown
Contributor

@angorayc angorayc left a comment

Choose a reason for hiding this comment

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

@nreese Thanks for the enhancement, LGTM!

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5470 5469 -1

Async chunks

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

id before after diff
maps 3.0MB 3.0MB +222.0B
securitySolution 15.1MB 15.1MB -1.4KB
total -1.1KB

Page load bundle

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

id before after diff
securitySolution 83.6KB 83.5KB -134.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 519 520 +1

References to deprecated APIs

id before after diff
securitySolution 522 521 -1

Total ESLint disabled count

id before after diff
securitySolution 601 602 +1

History

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

@nreese nreese merged commit 1c15e9a into elastic:main May 13, 2024
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR 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 t// v8.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants