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

Typescript map embeddable #61264

Merged

stacey-gammon merged 6 commits into elastic:master from stacey-gammon:2020-03-25-typescript-map-embeddable

Mar 26, 2020

Contributor

stacey-gammon commented Mar 25, 2020

Every time I refactor something in embeddables, I miss typescript in these files. Biting the bullet and converting them over to help me out with some of my future refactorings in the pipeline.

stacey-gammon force-pushed the 2020-03-25-typescript-map-embeddable branch from b666d90 to 2b06743 Compare

March 25, 2020 14:34

stacey-gammon added the [Deprecated-Use Team:Presentation]Team:Geo label

Contributor

elasticmachine commented Mar 25, 2020

Pinging @elastic/kibana-gis (Team:Geo)

stacey-gammon added chore release_note:skip v7.8 v8.0.0 labels

Contributor

nreese commented Mar 25, 2020

We are putting this on our roadmap for 7.8 so if you wanted to hold off, we could tackle this in the next few weeks

stacey-gammon force-pushed the 2020-03-25-typescript-map-embeddable branch from 2b06743 to bdcd5e0 Compare

March 25, 2020 14:48


typescript map embeddable

ce18bb1

stacey-gammon force-pushed the 2020-03-25-typescript-map-embeddable branch from bdcd5e0 to ce18bb1 Compare

March 25, 2020 14:53

stacey-gammon marked this pull request as ready for review

March 25, 2020 16:49

stacey-gammon requested a review from a team as a code owner

March 25, 2020 16:49

stacey-gammon requested a review from nreese

March 25, 2020 16:49

nreese reviewed

View reviewed changes

Contributor

nreese left a comment •

edited

Loading

Thanks for typescripting the MapEmbeddable

Here is some documentation that may clear up some typing questions https://github.com/elastic/kibana/blob/7.7/x-pack/legacy/plugins/maps/public/embeddable/README.md

x-pack/legacy/plugins/maps/public/actions/map_actions.d.ts Show resolved Hide resolved

x-pack/legacy/plugins/maps/public/actions/map_actions.d.ts Outdated


		export function setGotoWithCenter(config: MapCenter): AnyAction;

		export function replaceLayerList(layerList: unknown): AnyAction;

Contributor

nreese Mar 25, 2020

layerList is unknown[]

x-pack/legacy/plugins/maps/public/actions/map_actions.d.ts Outdated

+export interface RefreshConfig {
+ isPaused: boolean;
+ interval: unknown;

Contributor

nreese Mar 25, 2020

interval is a number

x-pack/legacy/plugins/maps/public/actions/map_actions.d.ts Outdated


		export function hideViewControl(hide: boolean): AnyAction;

		export function setHiddenLayers(layer: unknown): AnyAction;

Contributor

nreese Mar 25, 2020

Can you make this setHiddenLayers(hiddenLayerIds: string[])

x-pack/legacy/plugins/maps/public/actions/map_actions.d.ts Outdated Show resolved Hide resolved

x-pack/legacy/plugins/maps/public/actions/map_actions.d.ts Outdated Show resolved Hide resolved

x-pack/legacy/plugins/maps/public/actions/ui_actions.d.ts Outdated Show resolved Hide resolved

x-pack/legacy/plugins/maps/public/angular/get_initial_layers.d.ts Outdated Show resolved Hide resolved

x-pack/legacy/plugins/maps/public/connected_components/gis_map/index.d.ts Outdated


		import React from 'react';

		export const GisMap: React.FC<{ addFilters: unknown; renderTooltipContent: unknown }>;

Contributor

nreese Mar 25, 2020

addFilters: (filters[]) => void

renderTooltipContent: ({ addFilters, closeTooltip, features, isLocked, loadFeatureProperties}) => ReactComponent

Contributor

nreese Mar 25, 2020

What is React.FC?

Contributor Author

stacey-gammon Mar 26, 2020

Functional Component. I can just change it to Component. For renderToolTip params, since I don't know the types of all those params, I'm going to just do params: unknown for now.

Contributor Author

stacey-gammon Mar 26, 2020

actually I found the types in SIEM so moved them over


Address code review comments and update some usages in SIEM and uptim…

a809660

…e to the new types

stacey-gammon requested a review from a team as a code owner

March 26, 2020 13:52

Contributor Author

stacey-gammon commented Mar 26, 2020

addressed feedback @nreese. I also switched a couple places in SIEM and uptime that were using there own types to use these ones.

stacey-gammon requested a review from spong

March 26, 2020 13:54


More clean up - carry over some of the SIEM types to maps for render …

ee9bc9e

…tool tip

nreese reviewed

View reviewed changes

...legacy/plugins/uptime/public/components/functional/location_map/embeddables/embedded_map.tsx Show resolved Hide resolved


Merge branch 'master' of github.com:elastic/kibana into 2020-03-25-ty…

…pescript-map-embeddable

nreese reviewed

View reviewed changes

Contributor

nreese left a comment

thanks for all the updates.

x-pack/legacy/plugins/maps/public/embeddable/map_embeddable.tsx Outdated Show resolved Hide resolved

x-pack/legacy/plugins/maps/public/embeddable/map_embeddable.tsx Show resolved Hide resolved

x-pack/legacy/plugins/maps/public/embeddable/map_embeddable.tsx Outdated Show resolved Hide resolved

x-pack/legacy/plugins/maps/public/embeddable/types.ts Outdated Show resolved Hide resolved

x-pack/legacy/plugins/maps/public/embeddable/types.ts Outdated

+ features: MapFeature[];
+ isLocked: boolean;
+ getLayerName(layerId: string): Promise<string>;
+ loadFeatureProperties({ layerId, featureId }: LoadFeatureProps): Promise<FeatureProperty[]>;

Contributor

nreese Mar 26, 2020

return type is Promise<ITooltipProperty[]>, https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/maps/public/layers/tooltips/tooltip_property.ts#L10

Contributor Author

stacey-gammon Mar 26, 2020

This change is causing some big repercussions. Doesn't seem to match what Siem is passing in. I think I will switch back to unknown for this PR.

Contributor Author

stacey-gammon Mar 26, 2020

Contributor Author

stacey-gammon Mar 26, 2020

actually maybe if I do TooltipProperty it'll work, instead of ITooltipProperty

Contributor

nreese Mar 26, 2020

How about just unknown for loadFeatureProperties? Might as well define as much as possible.

Contributor Author

stacey-gammon Mar 26, 2020

I worked around it but had to use a cast to unknown in SIEM.

Member

spong Mar 26, 2020

This looks to be a by-product of SIEM accessing _propertyKey & _rawValue directly off of TooltipProperty (getPropertyKey() & getRawValue() interface wasn't around at the time). For this I think we can just update featureProps to be ITooltipProperty in MapToolTipComponent, LineToolTipContent, and PointToolTipContent, and then use getPropertyKey() & getRawValue() instead of accessing those props directly.

Feel free to leave the unknown @stacey-gammon and I can loop back to refactor after this PR is merged -- many thanks for helping clean things up here! 🙂

Though one question for @nreese -- I had typed _rawValue as string | string[], but I see it's string | undefined in ITooltipProperty. Iirc I was indeed seeing string[] for fields like host.ip and such within the tooltip -- should _rawValue and getRawValue() include string[] as well?

Contributor

nreese Mar 26, 2020

+1 for adding string[] to the value type

x-pack/legacy/plugins/maps/public/embeddable/types.ts Outdated Show resolved Hide resolved

x-pack/legacy/plugins/maps/public/selectors/map_selectors.d.ts Outdated Show resolved Hide resolved

x-pack/plugins/maps/public/index.ts Show resolved Hide resolved

x-pack/plugins/maps/public/reducers/non_serializable_instances.d.ts Outdated Show resolved Hide resolved


Address more review comments

d08da59

stacey-gammon requested a review from a team as a code owner

March 26, 2020 17:50


Merge branch 'master' of github.com:elastic/kibana into 2020-03-25-ty…

094deb4

…pescript-map-embeddable

Contributor

kibanamachine commented Mar 26, 2020

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 094deb4

History

💔 Build #36576 failed 6833276
💔 Build #36554 failed ee9bc9e
💔 Build #36540 failed a809660
💚 Build #36203 succeeded ce18bb1
💔 Build #36181 failed b666d90

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

nreese approved these changes

View reviewed changes

Contributor

nreese left a comment

Looks great. Thanks

lgtm
code review

spong reviewed

View reviewed changes

x-pack/legacy/plugins/siem/public/components/embeddables/map_tool_tip/map_tool_tip.tsx

@@ @@ -64,7 +64,7 @@ export const MapToolTipComponent = ({ @@
  getLayerName(layerId),
  ]);
- setFeatureProps(featureProperties);
+ setFeatureProps((featureProperties as unknown) as FeatureProperty[]);

Member

spong Mar 26, 2020

Commented on a potential cleanup here #61264 (comment), but since this involves additional SIEM refactoring I can take care of this in a follow PR, so feel free to leave as is. 👍

spong approved these changes

View reviewed changes

Member

spong left a comment

SIEM changes LGTM! 👍 Thanks @stacey-gammon! 🙂

Once merged, I'll loop back around to refactor a little more on the SIEM side as mentioned here.

stacey-gammon requested a review from crob611

March 26, 2020 20:13

Contributor Author

stacey-gammon commented Mar 26, 2020

Canvas changes are literally a one line import path change so I'm going to go ahead and merge. Thanks!

stacey-gammon merged commit 65452bd into elastic:master

stacey-gammon mentioned this pull request

[7.x] Typescript map embeddable (#61264) #61530

Merged

stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request


Typescript map embeddable (elastic#61264)

c980aa5

* typescript map embeddable

* Address code review comments and update some usages in SIEM and uptime to the new types

* More clean up - carry over some of the SIEM types to maps for render tool tip

* Address more review comments

crob611 mentioned this pull request

[Canvas] Use MapInput type from Maps plugin #61539

Merged

stacey-gammon added a commit that referenced this pull request


Typescript map embeddable (#61264) (#61530)

afcb00c

* typescript map embeddable

* Address code review comments and update some usages in SIEM and uptime to the new types

* More clean up - carry over some of the SIEM types to maps for render tool tip

* Address more review comments

rayafratkina added v7.8.0 and removed v7.8 labels

spong mentioned this pull request

[SIEM] [Maps] Feature tooltip content is empty #63474

Closed

spong mentioned this pull request

[SIEM] [Maps] Fixes Network Map empty tooltip #66828

Merged

1 task

spong added a commit that referenced this pull request


[SIEM] [Maps] Fixes Network Map empty tooltip (#66828)

951c0f6

## Summary

Resolves #63474, and expands `ITooltipProperty`'s `rawValue` type to include `string[]` as mentioned [here](#61264 (comment)).

![image](https://user-images.githubusercontent.com/2946766/82100568-2c0e1480-96c7-11ea-958e-5b1c6b6a3db9.png)



### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

spong added a commit to spong/kibana that referenced this pull request


[SIEM] [Maps] Fixes Network Map empty tooltip (elastic#66828)

1ef6505

## Summary

Resolves elastic#63474, and expands `ITooltipProperty`'s `rawValue` type to include `string[]` as mentioned [here](elastic#61264 (comment)).

![image](https://user-images.githubusercontent.com/2946766/82100568-2c0e1480-96c7-11ea-958e-5b1c6b6a3db9.png)



### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

spong added a commit that referenced this pull request


[SIEM] [Maps] Fixes Network Map empty tooltip (#66828) (#66945)

3dbe9dc

## Summary

Resolves #63474, and expands `ITooltipProperty`'s `rawValue` type to include `string[]` as mentioned [here](#61264 (comment)).

![image](https://user-images.githubusercontent.com/2946766/82100568-2c0e1480-96c7-11ea-958e-5b1c6b6a3db9.png)



### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

spong added a commit to spong/kibana that referenced this pull request


[SIEM] [Maps] Fixes Network Map empty tooltip (elastic#66828)

68b8baa

## Summary

Resolves elastic#63474, and expands `ITooltipProperty`'s `rawValue` type to include `string[]` as mentioned [here](elastic#61264 (comment)).

![image](https://user-images.githubusercontent.com/2946766/82100568-2c0e1480-96c7-11ea-958e-5b1c6b6a3db9.png)

### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

# Conflicts:
#	x-pack/plugins/maps/public/classes/fields/es_agg_field.ts
#	x-pack/plugins/maps/public/classes/fields/es_doc_field.ts
#	x-pack/plugins/maps/public/classes/fields/field.ts
#	x-pack/plugins/maps/public/classes/tooltips/es_tooltip_property.ts
#	x-pack/plugins/maps/public/classes/tooltips/join_tooltip_property.ts
#	x-pack/plugins/maps/public/classes/tooltips/tooltip_property.ts
#	x-pack/plugins/maps/public/index.ts
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/line_tool_tip_content.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/map_tool_tip.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/point_tool_tip_content.test.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/point_tool_tip_content.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/types.ts

spong added a commit to spong/kibana that referenced this pull request


[SIEM] [Maps] Fixes Network Map empty tooltip (elastic#66828)

11e6e0d

## Summary

Resolves elastic#63474, and expands `ITooltipProperty`'s `rawValue` type to include `string[]` as mentioned [here](elastic#61264 (comment)).

![image](https://user-images.githubusercontent.com/2946766/82100568-2c0e1480-96c7-11ea-958e-5b1c6b6a3db9.png)

### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

# Conflicts:
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/map_tool_tip.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/point_tool_tip_content.test.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/point_tool_tip_content.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/types.ts

spong added a commit that referenced this pull request


[SIEM] [Maps] Fixes Network Map empty tooltip (#66828) (#66955)

23c4889

## Summary

Resolves #63474, and expands `ITooltipProperty`'s `rawValue` type to include `string[]` as mentioned [here](#61264 (comment)).

![image](https://user-images.githubusercontent.com/2946766/82100568-2c0e1480-96c7-11ea-958e-5b1c6b6a3db9.png)

### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

# Conflicts:
#	x-pack/plugins/maps/public/classes/fields/es_agg_field.ts
#	x-pack/plugins/maps/public/classes/fields/es_doc_field.ts
#	x-pack/plugins/maps/public/classes/fields/field.ts
#	x-pack/plugins/maps/public/classes/tooltips/es_tooltip_property.ts
#	x-pack/plugins/maps/public/classes/tooltips/join_tooltip_property.ts
#	x-pack/plugins/maps/public/classes/tooltips/tooltip_property.ts
#	x-pack/plugins/maps/public/index.ts
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/line_tool_tip_content.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/map_tool_tip.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/point_tool_tip_content.test.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/point_tool_tip_content.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/types.ts

spong added a commit that referenced this pull request


[7.8] [SIEM] [Maps] Fixes Network Map empty tooltip (#66828) (#66956)

dc49b19

* [SIEM] [Maps] Fixes Network Map empty tooltip (#66828)

## Summary

Resolves #63474, and expands `ITooltipProperty`'s `rawValue` type to include `string[]` as mentioned [here](#61264 (comment)).

![image](https://user-images.githubusercontent.com/2946766/82100568-2c0e1480-96c7-11ea-958e-5b1c6b6a3db9.png)

### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

# Conflicts:
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/map_tool_tip.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/point_tool_tip_content.test.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/map_tool_tip/point_tool_tip_content.tsx
#	x-pack/plugins/siem/public/network/components/embeddables/types.ts

* Updating imports...

* And another

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore [Deprecated-Use Team:Presentation]Team:Geo release_note:skip v7.8.0 v8.0.0