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.

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] data load event handlers #49373

Merged
merged 6 commits into from
Oct 31, 2019
Merged

[Maps] data load event handlers #49373

merged 6 commits into from
Oct 31, 2019

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Oct 25, 2019

fixes #49236

This PR adds eventHandlers parameter to MapEmbeddableFactory.createFromState allowing consumers to register callbacks for onDataLoad, onDataLoadEnd, onDataLoadError.

const eventHandlers = {
  onDataLoad: (layerId: string, dataId: string) => {
    // take action on data load
  },
  onDataLoadEnd: (layerId: string, dataId: string, featuresCount: number) => {
    // take action on data load end
  },
  onDataLoadError: (layerId: string, dataId: string, errorMessage: string) => {
    // take action on data load error
  },
}

const mapEmbeddable = await factory.createFromState(state, input, parent, renderTooltipContent, eventHandlers);

@nreese nreese added release_note:enhancement [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.6.0 labels Oct 25, 2019
@nreese nreese requested a review from spong October 25, 2019 18:05
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spong
Copy link
Member

spong commented Oct 28, 2019

@elasticmachine merge upstream

@spong
Copy link
Member

spong commented Oct 29, 2019

Here's a draft PR using this new API: #49589

To meet our use case I created a useMapLayers hook that provides both a global isLoading flag and an array of layer state. Having a global loading flag makes things a bit easier to digest on our end.

Anyway, implementation was pretty seamless (thank-you! 🙂) , only two pieces of feedback:

  • The root layer is included within onDataLoad but not onDataLoadEnd. I was able to filter it out to get the desired behavior, but I'm thinking it either shouldn't be there, or also be included in onDataLoadEnd. Thoughts?
  • Small naming discrepancy in the API description above for the onDataLoadEnd props -- featureCount (desc) vs featuresCount (as named in code).

Other than that everything looks 👍 Thanks for the added functionality and quick turn-around on this!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese
Copy link
Contributor Author

nreese commented Oct 29, 2019

Small naming discrepancy in the API description above for the onDataLoadEnd props -- featureCount (desc) vs featuresCount (as named in code).

Thanks, fixed

The root layer is included within onDataLoad but not onDataLoadEnd. I was able to filter it out to get the desired behavior, but I'm thinking it either shouldn't be there, or also be included in onDataLoadEnd. Thoughts?

There was an uncaught error "Uncaught (in promise) TypeError: Cannot read property 'length' of undefined" resulting in onDataLoadEnd not getting called for that layer. That issue has been resolved.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Updated #49589 against the latest from this PR and no longer seeing an error in the console and onDataLoadEnd now gets called for the EMS layer. LGTM -- thanks @nreese! 👍

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

Awesome. One small piece of feedback:

I would wrap the 3rd argument of the callback, featuresCount, into an object. The contents of this argument depend on the layer-type (e.g. tile-layer could have different "completion"-arguments than a heatmap-layer than a vector-layer). featuresCount only applies to vector-layer.

wrt @spong

Having a global loading flag makes things a bit easier to digest on our end.

I'd agree with that, but I'd hesitate to put this global-flag into a first pass at this. It's hard to create a global data-load flag and have it work reliably in Maps. It's not just the data-loading itself, but detecting map-movements, tile-loading, ... those would all have to be wrapped up into this.

@nreese
Copy link
Contributor Author

nreese commented Oct 30, 2019

@thomasneirynck @spong featuresCount is now wrapped in resultMeta object so different layer types can return different results. featuresCount is only provided for vector layers

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Updated #49589 against the latest that introduces the resultMeta object and all is good -- LGTM 👍

@nreese nreese merged commit 70c0583 into elastic:master Oct 31, 2019
nreese added a commit to nreese/kibana that referenced this pull request Oct 31, 2019
* [Maps] data load event handlers

* update readme featuresCount to reflect variable name

* fix exception with EMS_TMS onDataLoadEnd

* wrap featuresCount in resultMeta object so different layer types can return different results
nreese added a commit that referenced this pull request Oct 31, 2019
* [Maps] data load event handlers

* update readme featuresCount to reflect variable name

* fix exception with EMS_TMS onDataLoadEnd

* wrap featuresCount in resultMeta object so different layer types can return different results
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] [Embeddables] Provide ability to retrieve number of features for a given layer
4 participants