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] Add ordering to layer wizard registration API #122999

Merged
merged 7 commits into from
Jan 14, 2022

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Jan 13, 2022

The layer-wizard registration API is exposed to other plugins.

There is no guarantee of the order in which the wizards are displayed in the UX, because there is no guarantee in the order in which plugins will call this API. e.g. they might even call it before the Maps-app registers its own layer-wizards.

The practical result is that layer-wizards from other plugins would end up "on top", where the Maps-app would still want to surface its own wizards first (e.g. upload GeoJson-wizard should come on top).

This was revealed in #122862.

By default, the ordering is set to 0.

Clients using the registerLayerWizard API would have to set the order param. This will allow us to enforce an ordering of layer-wizards.

e.g. in #122862, we'd set the order: 100 so the wizard does not surface on top.

@thomasneirynck thomasneirynck added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes v8.1.0 labels Jan 13, 2022
@thomasneirynck thomasneirynck requested a review from a team as a code owner January 13, 2022 20:20
@elasticmachine
Copy link
Contributor

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

LAYER_WIZARD_CATEGORY,
MAX_ZOOM,
MIN_ZOOM,
VECTOR_SHAPE_TYPE,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

piggy-backing of this PR, but not really related to the ordering-issue. But the export of these types is required for #122862 as well

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

LGTM ⚡ 🙏

@@ -39,11 +39,13 @@ export type LayerWizard = {
renderWizard(renderWizardArguments: RenderWizardArguments): ReactElement<any>;
title: string;
showFeatureEditTools?: boolean;
order?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should order be required? Should we set order for existing maps wizards? Now order is registration order. Maybe this would be better coded with values. Maybe maps plugins should start at 100 and manually set numbers above that. Then plugins could use 0-99 to register before maps wizards ad 200+ to register after maps plugins.

Copy link
Contributor Author

@thomasneirynck thomasneirynck Jan 13, 2022

Choose a reason for hiding this comment

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

after discussion:

See proposal:

  • 0-99: reserved for maps plugin
    • 10: maps-specific wizards
    • 20: solution layers
  • 100+: non-Maps client plugins

Happy to make changes/tweaks

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
code review

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
maps 207 213 +6

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 +153.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
maps 30 28 -2

Page load bundle

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

id before after diff
maps 36.7KB 37.1KB +373.0B
Unknown metric groups

API count

id before after diff
maps 208 214 +6

History

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

@thomasneirynck thomasneirynck merged commit 0675a6a into elastic:main Jan 14, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 14, 2022
ogupte pushed a commit to ogupte/kibana that referenced this pull request Jan 28, 2022
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:skip Skip the PR/issue when compiling release notes v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants