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 shape drawing wizard- creates index only #96913

Closed
wants to merge 70 commits into from

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Apr 13, 2021

Overview

Adds the wizard capability described in #96836. To run and test:

  1. Enable by adding the following to your kibana.yml:
xpack.maps.enableDrawingFeature: true
  1. Open Maps > Add layer > Draw new vector layer
  2. Enter an index name
  3. Click Index features at the bottom right
  4. That's it! This PR doesn't add features, it just creates an index and index pattern. Since there's no visual evidence this happened, you can also check that both were created via Stack management

A few open questions

  • This layer type is currently 2nd in the menu next to GeoJSON Upload, and like GeoJSON Upload it's a low barrier-to-entry layer-add option. That said, it can go anywhere (1st, 2nd, last, etc.) if there are any strong opinions.
    image
  • For the Draw vector shapes portion, we likely need something giving the user some instruction here, but I'm very open to what that looks like and consists of. @miukimiu @kmartastic
    image
  • What do we want to name this feature? Should we call it: Draw shapes, Draw vector shapes, Draw, something else? Not critical, we can change this in a later PR before pulling out the feature flag.

Notes

Aaron Caldwell added 30 commits March 26, 2021 15:50
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.

thanks! some comments for cleanup and possible simplification of the wizard, otherwise good2go imho

x-pack/plugins/maps/public/actions/map_action_constants.ts Outdated Show resolved Hide resolved
export const newVectorLayerWizardConfig: LayerWizard = {
categories: [],
description: i18n.translate('xpack.maps.newVectorLayerWizard.description', {
defaultMessage: 'Draw points, lines, and polygons to create or edit documents',
Copy link
Contributor

Choose a reason for hiding this comment

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

my 2c the wording should be more generic, but @kmartastic has final call here:

more along the lines of:

title: Create new document layer

and

description: ```Creates a new index pattern. Can be used to draw shapes and points`.

or something.

We can work on the more integrated wording, when we actually integrate the flow after this PR and the one for the drawing toolbar have merged. But right now, that integration is still todo, so it's a little confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, just a little reluctant to touch the wording again on this PR after we got to somewhat of a settling point. I think we'll be iterating on this before pulling out of experimental mode so no need for it to be final at this point. Let me know if you feel strongly though

Copy link
Contributor

Choose a reason for hiding this comment

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

let's change it, so this PR can standalone and we're not priming it for hypothetical later work.

I'd propose:

  • "title": "Create new document layer"
  • "description": Creates a new empty index pattern. Use this to draw shapes and point".

^ we can change once we start pulling threads together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep the title shorter? Most of the other card titles are a single word. I really like "draw" so something short.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to a shorter title, which makes it easier to scan.

For the description how about shortening to:

Create an empty index pattern to draw shapes and points.

Also, aren't index patterns being renamed in 7.14?

x-pack/plugins/maps/public/reducers/map/map.ts Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/reducers/map/map.ts Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/reducers/map/types.ts Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/reducers/map/types.ts Outdated Show resolved Hide resolved
</Fragment>
}
/>
<IndexNameForm
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass in the below props give users feedback when name is getting validated

onIndexNameValidationEnd={this.props.enableNextBtn}
onIndexNameValidationStart={this.props.disableNextBtn}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thinks this logic needs some rethinking on the IndexNameForm side but not sure it fits in this PR. Using that logic above enables the button even when there are errors. Even adding an if statement to check for errors on the callback runs into timing issues that leave the button active with errors in place:

() => {
  if (this.state.indexName && !this.state.indexError) {
    this.props.enableNextBtn();
  }
}

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.

some small comments, otherwise good2go from my end.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fileUpload 159 161 +2
maps 773 778 +5
total +7

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 190 192 +2

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.7MB 2.7MB +5.9KB

Page load bundle

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

id before after diff
fileUpload 21.5KB 23.0KB +1.5KB
maps 62.1KB 62.6KB +493.0B
total +2.0KB
Unknown metric groups

API count

id before after diff
maps 191 193 +2

History

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

@thomasneirynck
Copy link
Contributor

Closing this. Replaced by #100278.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation enhancement New value added to drive a business result release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants