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

[Lens] visualize in maps button #98677

Merged
merged 28 commits into from
May 12, 2021
Merged

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Apr 28, 2021

Closes #80078

Show geo_point and geo_shape fields in Lens field list. Add visualize in maps button in field details. PR also shows a custom workspace panel when users drag geo fields into the workspace. The workspace panel has a drop zone that when the field is dropped into the zone, takes users to the maps application.

lensToMaps

@nreese nreese added WIP Work in progress [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 Feature:Lens v7.14.0 reason:enhancement labels Apr 28, 2021
@nreese nreese requested a review from a team April 28, 2021 19:49
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

I know this is in draft, so I've left the smallest review I can.

@flash1293
Copy link
Contributor

@nreese @wylieconlon I wrote down an idea how to implement the second part of this feature (custom workspace drop target that leads to maps): https://gist.github.com/flash1293/3a55232eb1f0162125ad3aa8e1ac3f99

@timroes
Copy link
Contributor

timroes commented Apr 29, 2021

I wonder if we really want to show that geo point if there would be no action available for that trigger, or if we should already make listing those fields (and not just disabling the button within the popover) dependant on whether there is an action available?

@flash1293
Copy link
Contributor

Agree with that @timroes - if the action is not available we should hide the field in the list.

@nreese
Copy link
Contributor Author

nreese commented Apr 29, 2021

@elasticmachine merge upstream

@nreese nreese requested a review from a team as a code owner April 30, 2021 20:46
@nreese
Copy link
Contributor Author

nreese commented May 3, 2021

@elasticmachine merge upstream

@nreese nreese requested a review from wylieconlon May 3, 2021 20:37
@nreese nreese removed the WIP Work in progress label May 3, 2021
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Looks pretty good already. Do you need some help to polish the UI? Or did you plan to do that after the general feature is working?

One thing we should probably think about: If the user came to Lens from dashboard and goes to maps via the action, the connection to dashboard is lost and the maps "Save" button will prompt them to select a dashboard again. Are we fine with that? Or should we find a way to pass this information along?

@nreese
Copy link
Contributor Author

nreese commented May 4, 2021

Do you need some help to polish the UI?

I will need help creating the icon from the screen shots in the demo. Also, any other suggestions would be helpful as well but I have mainly been focused on getting the functionality working

@flash1293
Copy link
Contributor

@cchaos or @MichaelMarcialis Can you assist getting the icon out of the mockups here? #80078

@nreese
Copy link
Contributor Author

nreese commented May 5, 2021

UI looks good except for the drop zone (not centered and a bit too small in both height and width)

Resolved

This kind of navigation is a prime candidate to break because of unrelated changes, do you think it would make sense adding a small functional test for this (either to the Lens or the maps suite)? Just to check whether the maps app is loaded correctly.

Functional test added to lens suite

One thing we should probably think about: If the user came to Lens from dashboard and goes to maps via the action, the connection to dashboard is lost and the maps "Save" button will prompt them to select a dashboard again. Are we fine with that? Or should we find a way to pass this information along?

I am fine having this work done in a separate PR since it could balloon in scope.

@nreese nreese requested a review from flash1293 May 5, 2021 22:27
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Looks almost good to me except for the a11y message when dragging to the new drop zone. The best solution I can think of (maybe someone else has a better idea) is to extend HumandData by something like specialCallout?: string which overwrites the default announcement and then setting it to something like Visualize {field} in Maps

const dragDropIdentifier = {
id: 'lnsGeoFieldWorkspace',
humanData: {
label: i18n.translate('xpack.lens.geoFieldWorkspace.workspaceLabel', {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is used to construct the screen reader message if the user picks up the field using keyboard and moves it to the drop zone:
Screenshot 2021-05-06 at 15 51 38

We can either add a special case for this here (extending humanData for a special message):

: i18n.translate('xpack.lens.dragDrop.announce.selectedTarget.defaultNoPosition', {

or think about something that fits into the existing template and makes clear this will move the user to Maps.

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Left some small comments and suggestions below. Also going to send along an updated SVG globe graphic. Noticed some oddities in the original illustration that I'd like to correct. Stay tuned.

nreese and others added 6 commits May 10, 2021 07:52
…orkspace_panel/geo_field_workspace_panel.tsx

Co-authored-by: Michael Marcialis <[email protected]>
…orkspace_panel/geo_field_workspace_panel.scss

Co-authored-by: Michael Marcialis <[email protected]>
…orkspace_panel/geo_field_workspace_panel.tsx

Co-authored-by: Michael Marcialis <[email protected]>
…orkspace_panel/geo_field_workspace_panel.scss

Co-authored-by: Michael Marcialis <[email protected]>
…orkspace_panel/geo_field_workspace_panel.scss

Co-authored-by: Michael Marcialis <[email protected]>
@nreese
Copy link
Contributor Author

nreese commented May 10, 2021

@elasticmachine merge upstream

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Left one small comment below. Otherwise, looks good to me. Approving now so I don't hold you up.

nreese and others added 2 commits May 10, 2021 15:16
…orkspace_panel/geo_field_workspace_panel.tsx

Co-authored-by: Michael Marcialis <[email protected]>
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM - tested and works fine. Thanks for this!

@nreese
Copy link
Contributor Author

nreese commented May 12, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 496 508 +12

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 970.4KB 999.5KB +29.0KB

Page load bundle

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

id before after diff
lens 35.1KB 35.1KB +40.0B
Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 4 2 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 5 3 -2
licensing 18 15 -3
monitoring 109 56 -53
total -73

History

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

@nreese nreese merged commit b097385 into elastic:master May 12, 2021
nreese added a commit to nreese/kibana that referenced this pull request May 12, 2021
* [Lens] visualize in maps button

* clean up dependency injection as suggested

* add custom workspace render for geo fields

* tslint and finish drag and drop for geo field

* convert react class to function component

* prevent page reload when clicking visualize in maps button

* filter allFields instead of using condition to populate fieldTypeNames to fix tslint

* clean up UI

* fix jest test

* globe illustration

* UI cleanup

* functional test

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/geo_field_workspace_panel.tsx

Co-authored-by: Michael Marcialis <[email protected]>

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/geo_field_workspace_panel.scss

Co-authored-by: Michael Marcialis <[email protected]>

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/geo_field_workspace_panel.tsx

Co-authored-by: Michael Marcialis <[email protected]>

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/geo_field_workspace_panel.scss

Co-authored-by: Michael Marcialis <[email protected]>

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/geo_field_workspace_panel.scss

Co-authored-by: Michael Marcialis <[email protected]>

* Update x-pack/plugins/lens/public/indexpattern_datasource/field_item.tsx

Co-authored-by: Michael Marcialis <[email protected]>

* updated globe svg

* remove unused

* better message for drop zone screen reader

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/geo_field_workspace_panel.tsx

Co-authored-by: Michael Marcialis <[email protected]>

* tslint

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Michael Marcialis <[email protected]>
nreese added a commit that referenced this pull request May 12, 2021
* [Lens] visualize in maps button

* clean up dependency injection as suggested

* add custom workspace render for geo fields

* tslint and finish drag and drop for geo field

* convert react class to function component

* prevent page reload when clicking visualize in maps button

* filter allFields instead of using condition to populate fieldTypeNames to fix tslint

* clean up UI

* fix jest test

* globe illustration

* UI cleanup

* functional test

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/geo_field_workspace_panel.tsx

Co-authored-by: Michael Marcialis <[email protected]>

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/geo_field_workspace_panel.scss

Co-authored-by: Michael Marcialis <[email protected]>

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/geo_field_workspace_panel.tsx

Co-authored-by: Michael Marcialis <[email protected]>

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/geo_field_workspace_panel.scss

Co-authored-by: Michael Marcialis <[email protected]>

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/geo_field_workspace_panel.scss

Co-authored-by: Michael Marcialis <[email protected]>

* Update x-pack/plugins/lens/public/indexpattern_datasource/field_item.tsx

Co-authored-by: Michael Marcialis <[email protected]>

* updated globe svg

* remove unused

* better message for drop zone screen reader

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/geo_field_workspace_panel.tsx

Co-authored-by: Michael Marcialis <[email protected]>

* tslint

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Michael Marcialis <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Michael Marcialis <[email protected]>
@tsullivan
Copy link
Member

Hi! I'm just popping in after doing some 7.14 testing. I like that the field is shown in the listing and can be accessed as a way to navigate to Maps: it's a nice, streamlined process users may even learn to access intentionally.

Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Support for geo fields / maps
10 participants