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] Register GeoJson upload with integrations page #126350

Merged
merged 21 commits into from Mar 14, 2022

Conversation

maksimkovalev
Copy link
Contributor

@maksimkovalev maksimkovalev commented Feb 24, 2022

Closes: #116653

Added cards on Integration page to upload GeoJson. Links to Maps app with upload_geojson parameter in url now opens layer sidebar with expanded wizard.

Screenshot 2022-03-04 at 18 00 08

@maksimkovalev maksimkovalev added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:enhancement v8.2.0 labels Feb 24, 2022
@maksimkovalev
Copy link
Contributor Author

@thomasneirynck would you like to review this PR?

@nreese nreese added the backport:skip This commit does not require backporting label Feb 28, 2022
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

I think that layerWizardId needs to be renamed to something more specific. As is, layerWizardId is very misleading as it conveys that it's the redux state for any open layer wizard and this is not the case. In AddLayerPanel, selecting a layer wizard only updates component state and does not update redux state. I think renaming layerWizardId to something like autoOpenLayerWizardId will avoid confusion.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Can you add a functional test to ensure the URL works. https://github.com/elastic/kibana/blob/main/x-pack/test/functional/apps/maps/discover.js is a good example of testing linking to maps

x-pack/plugins/maps/common/constants.ts Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/actions/ui_actions.ts Outdated Show resolved Hide resolved
x-pack/plugins/maps/server/register_integrations.ts Outdated Show resolved Hide resolved
@nreese
Copy link
Contributor

nreese commented Mar 9, 2022

@elasticmachine merge upstream

@nreese nreese requested a review from gchaps March 9, 2022 16:27
@nreese nreese marked this pull request as ready for review March 9, 2022 16:27
@nreese nreese requested a review from a team as a code owner March 9, 2022 16:27
@elasticmachine
Copy link
Contributor

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

defaultMessage: 'Upload GeoJson with Elastic Maps.',
}),
uiInternalPath: core.http.basePath.prepend(
getFullPath(`?${OPEN_LAYER_WIZARD}=${WIZARD_ID.GEO_FILE}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

The signature of getFullPath expects a saved object id. We are kind of hijacking the method here. Plus the URL is not quite right and when followed produces URLs like http://localhost:5601/qni/app/maps/map?openLayerWizard=uploadGeoFile#?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))&_a=(filters:!(),query:(language:kuery,query:''))

Notice there are 2 ? which is wrong.

You can change to the below. It looks like core.http.basePath.prepend is not needed. Also notice # character added. Query parameters need to be after # character.

uiInternalPath: `${getFullPath()}#?${OPEN_LAYER_WIZARD}=${WIZARD_ID.GEO_FILE}`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you mean getFullPath('')? Because this function requires an id or undefined.

x-pack/plugins/maps/server/register_integrations.ts Outdated Show resolved Hide resolved
@nreese
Copy link
Contributor

nreese commented Mar 10, 2022

@elasticmachine merge upstream

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM. Lets get copy review from @gchaps before merging
code review, tested in chrome

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
maps 758 759 +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 2.5MB 2.5MB +1.2KB

Page load bundle

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

id before after diff
maps 65.6KB 66.2KB +661.0B

History

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

@nreese nreese merged commit 56c38b6 into elastic:main Mar 14, 2022
maksimkovalev added a commit to maksimkovalev/kibana that referenced this pull request Mar 18, 2022
* elastic#116653 - added GeoJson upload for Intergrations page

* 116653 - refactoring for messages

* 116653 - fixed Checks and api tests

* 116653 - fixed checks

* 116653 - refactoring; Added constants and registrations for shapefile

* 116653 - fixed api test

* 116653 - added method for getting selected wizard

* 116653 - refactoring

* 116653 - refactoring; Added constants, variables renamed

* 116653 - prettier fixes

* 116653 - refactoring; functional test added

* 116653 - test file extension changed to TS

* 116653 - refactoring

* 116653 - refactoring; test updated

* 116653 - refactoring

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:enhancement v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] Register GeoJson upload with integrations page
6 participants