-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Maps] Convert AbstractSource and AbstractLayer to TS #63533
[Maps] Convert AbstractSource and AbstractLayer to TS #63533
Conversation
Pinging @elastic/kibana-gis (Team:Geo) |
case HeatmapLayer.type: | ||
return new HeatmapLayer({ layerDescriptor, source }); | ||
case BlendedVectorLayer.type: | ||
return new BlendedVectorLayer({ layerDescriptor, source }); | ||
return new BlendedVectorLayer({ layerDescriptor, source, joins: [] }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does BlendedVectorLayer take joins? BlendedVectorLayer does not support joins so this seams like a strange thing to expose in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consequence of requiring VectorLayerArguments
as param. Can create new param-type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or just make joins
optional?
x-pack/plugins/maps/common/descriptor_types/style_property_descriptor_types.d.ts
Show resolved
Hide resolved
x-pack/plugins/maps/public/layers/sources/es_geo_grid_source/es_geo_grid_source.d.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/maps/common/descriptor_types/descriptor_types.d.ts
Outdated
Show resolved
Hide resolved
Thank you so much for doing all of this! Just left some minor comments |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
x-pack/plugins/maps/common/descriptor_types/descriptor_types.d.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/maps/common/descriptor_types/descriptor_types.d.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/maps/common/descriptor_types/descriptor_types.d.ts
Outdated
Show resolved
Hide resolved
@@ -20,6 +20,7 @@ export interface IESSource extends IVectorSource { | |||
limit: number, | |||
initialSearchContext?: object | |||
): Promise<SearchSource>; | |||
loadStylePropsMeta(args: unknown): Promise<unknown>; // todo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
args are
layerName: string,
style: VectorStyle,
dynamicStyleProps: IDynamicStyleProperty[],
registerCancelCallback: (requestToken: symbol, callback: () => void): void,
nextMeta: VectorSourceRequestMeta
return type is
RangeFieldMeta | CategoryFieldMeta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the signature. the return type is actually a raw ES response. Will leave typing of that for another PR, since it bloats scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
code review
@@ -0,0 +1,60 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this file be renamed to style.ts
since it contains IStyle
@@ -60,6 +60,7 @@ export class VectorStyle extends AbstractStyle { | |||
|
|||
constructor(descriptor = {}, source, layer) { | |||
super(); | |||
descriptor = descriptor === null ? {} : descriptor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed since function arguments defaults descriptor to {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a null
value doesn't fallback to the default
I'm unable to reproduce these functional test errors locally (https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/41853/). ? re-basing with master again ? |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Converts source and layer to .ts modules.
This will greatly help navigation in IDEs, and make extending easier.