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 support for geohex_grid aggregation #127170

Merged
merged 28 commits into from
Mar 23, 2022
Merged

[Maps] Add support for geohex_grid aggregation #127170

merged 28 commits into from
Mar 23, 2022

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Mar 8, 2022

This PR updates cluster layers with hexagons.

documentation updates https://kibana_127170.docs-preview.app.elstc.co/diff

Screen Shot 2022-03-18 at 9 16 31 AM

Cluster layers are added to map by selecting "Clusters" wizard. Notice copy and icon updates.
Screen Shot 2022-03-18 at 9 15 33 AM

Users now have option to select Hexagons to display clusters as hexagons. Tooltip added to describe options and provide disabled message. Hexagons only support geo_point fields. Hexagon aggregation is licensed basic in cloud and gold in on prem.
Screen Shot 2022-03-18 at 9 15 56 AM

Screen Shot 2022-03-18 at 9 16 01 AM

@mdefazio
Copy link
Contributor

mdefazio commented Mar 9, 2022

@nreese Following from our conversation on Slack, I put together a few options for the 'Show as' configuration. I didn't dig too far into this, but based on the following criteria I put together 2 options to consider.

  • Need to explain why a field is no longer available (based on field type)
  • Would like to allow user to discover new options for displaying clusters, even if not in their license level.

Options:

Perhaps just a simple tooltip could work so users might understand why an option is no longer available. The copy could likely be improved upon, but just showing the overall idea.

image


Could also consider simply showing as radio buttons. This will give more flexibility in what you want to show and perhaps also afford the option to guide the user which option is best. We could also include the tooltip option above for this as well.

image

Again, just some quick thoughts and happy to dig more into this—i'm guessing there's a few states of this form that I'm not considering.

@nreese
Copy link
Contributor Author

nreese commented Mar 14, 2022

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Mar 15, 2022

@mdefazio I decided to go with the first option and provide a tooltip on the label. What do you think?

Screen Shot 2022-03-15 at 9 59 14 AM

@mdefazio
Copy link
Contributor

@nreese With this much copy, I think it's best to have this be a popover as opposed to a tooltip so the user doesn't have to keep their cursor stationary while reading. The link is also likely more visible/likely to be clicked on than the tooltip icon. If we want to stick with the tooltip, perhaps shortening this and then using the right side link to go to docs for these options?

Is the concern that users won't know the difference between them? Or why some options are available depending on the field selected? Do we need to describe each with this much detail? With the popover, we could try and provide a small visualization next to each.

I think you could also keep line 'Hexbins require geo_point field' visible as help text for the button group, again assuming the requirement here is to let users know why an option is disabled.

Here's a quick mockup using the popover. Popover title is maybe not necessary.

Screen Recording 2022-03-15 at 01 30 00 PM

@nreese
Copy link
Contributor Author

nreese commented Mar 15, 2022

Component is also displayed in column mode. So popover will need to stick with an icon but I can switch to a popover

Screen Shot 2022-03-15 at 11 56 38 AM

@mdefazio
Copy link
Contributor

If we trim down the copy, I think the tooltip is fine (and we then also stick with exisiting patterns for the label + tooltip). My main concern is just the length of that copy.

@nreese
Copy link
Contributor Author

nreese commented Mar 15, 2022

If we trim down the copy, I think the tooltip is fine (and we then also stick with exisiting patterns for the label + tooltip). My main concern is just the length of that copy.

What would you trim?

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

This is a stab at updating the copy—hopefully i'm not changing definitions here, but trying to get them into single sentences.

@nreese
Copy link
Contributor Author

nreese commented Mar 17, 2022

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Mar 17, 2022

This is a stab at updating the copy—hopefully i'm not changing definitions here, but trying to get them into single sentences.

Thanks I have incorporated the suggestions.

@nreese nreese added release_note:enhancement [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation backport:skip This commit does not require backporting v8.2.0 labels Mar 17, 2022
@nreese
Copy link
Contributor Author

nreese commented Mar 18, 2022

@elasticmachine merge upstream

@nreese nreese requested a review from gchaps March 18, 2022 18:51
Copy link
Contributor

@gchaps gchaps left a comment

Choose a reason for hiding this comment

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

LGTM with addition of a link to the subscription page.

if (!getIsCloud() && !getIsGoldPlus()) {
isHexDisabled = true;
hexDisabledReason = i18n.translate('xpack.maps.hexbin.license.disabledReason', {
defaultMessage: '{hexLabel} is a subscription feature.',
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be:

xxx is a subscription feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should links be in tooltips? There is no way to click the link.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. Do you have a quick screenshot of where this copy is? Can see if there are maybe some other options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-03-18 at 3 54 23 PM

@nreese
Copy link
Contributor Author

nreese commented Mar 21, 2022

@elasticmachine merge upstream

@nreese nreese marked this pull request as ready for review March 21, 2022 14:04
@nreese nreese requested a review from a team as a code owner March 21, 2022 14:04
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@nickpeihl nickpeihl 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 and tested in chrome.

@nreese
Copy link
Contributor Author

nreese commented Mar 22, 2022

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Mar 22, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
maps 773 777 +4

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 +3.4KB

Page load bundle

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

id before after diff
maps 68.2KB 68.4KB +193.0B

History

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

@nreese nreese merged commit 75f8ac4 into elastic:main Mar 23, 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:enhancement v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants