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.

Sign up for GitHub

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

Jump to bottom

[Maps] show dynamic style ranges in legend #35417

Merged
merged 40 commits into from
May 7, 2019
Merged

[Maps] show dynamic style ranges in legend #35417

merged 40 commits into from
May 7, 2019

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Apr 22, 2019 •
edited
Loading

Screen Shot 2019-05-02 at 8 41 00 AM

This PR:

  • Clicking on layer title now opens/closes layer actions context menu
  • Add "Edit Layer" to layer action context menu
  • Move layer sort icon from always being visible to only being visible when layer title has hover event.
  • Update layer icon to show middle color ramp color for dynamic fill color and border color.
  • Display all dynamic style ranges in layer details
  • Save/restore visible layer details in Saved object and embeddable config so users can show/hide layer details and have their choices preserved between sessions.

@nreese nreese requested a review from a team as a code owner April 22, 2019 16:46
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese nreese added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:enhancement v7.2.0 v8.0.0 labels Apr 23, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

thomasneirynck
thomasneirynck reviewed May 3, 2019
View reviewed changes
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.

This is a massive PR, and has so many new features. 💫

  • improves the small legend icon in the left of the TOC
  • shows a detailed legend on expansion
  • adds range readouts
  • changes the layout of the TOC-entry, making it more intuitive to activate edit-mode
  • making legend expandable, and savind this per map and per dashboard (really neat!)
  • cleanup of some of the icon-rendering code, simplifying some of the constructs

I'm sure I'm forgetting more. 🚀

I have some minor comments in the code. It was a lot, so I might have not really thorough.

A couple minor comments.

  • Maybe trim the range readout for longer values?
    image

  • I think the general layout gets a little cluttered. It's hard to see which legend applies to which value. But I defer to @cchaos and @alexfrancoeur here.

e.g. Is the count the color ramp or the bubble-size?
image

x-pack/test/functional/page_objects/gis_page.js Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/shared/layers/styles/abstract_style.js Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/shared/layers/styles/vector_style.js Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/components/widget_overlay/layer_control/layer_toc/toc_entry/view.js Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/shared/layers/styles/components/vector/legend/index.js Outdated Show resolved Hide resolved
@nreese
Copy link
Contributor Author

nreese commented May 3, 2019

I think the general layout gets a little cluttered. It's hard to see which legend applies to which value

The field name has a tooltip that displays the full name if the name gets truncated and contains the style property so this will alleviate help avoid confusion. I would rather avoid taking up more vertical space for each scaled property so the user can display more data and not have to scroll the legend

Screen Shot 2019-05-03 at 10 06 46 AM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese nreese requested a review from thomasneirynck May 3, 2019 18:29
@kindsun
Copy link
Contributor

kindsun commented May 6, 2019

Looking really nice! A couple of observations:

Seems like we should consider either not showing range details for not-currently-visible layers or showing them but updating range metadata on a different event sequence than the one currently used (initiated by stopLoading in store_actions. Currently when we load the map with layers that aren't visible, we get everything but the range details (see taxful_total_price below):

image

The last-used expand button keeps the focus after clicking and leaving the Legend Entry. Maybe we should consider changing/clearing the focus after leaving it:

Peek 2019-05-06 08-51

@cchaos
Copy link
Contributor

cchaos commented May 6, 2019

The last-used expand button keeps the focus after clicking and leaving the Legend Entry. Maybe we should consider changing/clearing the focus after leaving it

In your screenshot, the focus will continue to be on the legend toggle until the user clicks somewhere else. This is by design so that these buttons (that are hidden until hover) will show when a keyboard user is tabbing through them. It's the default behavior of a browser to keep focus on the last clicked item. I would not change this default behavior.

@nreese
Copy link
Contributor Author

nreese commented May 7, 2019

Seems like we should consider either not showing range details for not-currently-visible layers

I think I will leave them visible to avoid having the TOC expand/collapse as users zoom around the map.

@nreese nreese requested a review from thomasneirynck May 7, 2019 18:03
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

thomasneirynck
thomasneirynck approved these changes May 7, 2019
View reviewed changes
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.

🐎

@nreese nreese merged commit 206dda7 into elastic:master May 7, 2019
@nreese nreese mentioned this pull request May 7, 2019
Merged
nreese added a commit to nreese/kibana that referenced this pull request May 7, 2019
@nreese
[Maps] show dynamic style ranges in legend (elastic#35417)
446ea01 
* show grab and edit buttons when hovering over layer name

* display layer details when layer name is clicked

* add range to dynamic style property state

* render dynamic ranges in TOC details

* render symbol size and border width header in legend

* simplify VectorStyle.getIcon

* removed unused component

* open TOC details on map load

* save open TOC details state in embeddable config

* show gradients for dynmaic fill color icon

* round corners of dynamic icon if points only

* add tooltip to legend label

* add edit panel action to action panel

* add functional test for details in legend

* fix broken gis_page function doesLayerExist

* add unit tests for VectorStyle.getDescriptorWithDynamicRanges

* open actions menu on layer title click, add arrow up/down for togging layer details

* Design cleanup of layers panel (#31)

* fix functional test

* update jest snapshots

* refactor StylePropertyLegendRow to use same function to render lineWidth and iconSize legends

* fix functional test

* fix another functional test

* make escapeLayerName function instead of instance method

* move _isLayerDetailsOpen into prop from redux connector

* remove index.js file

* do not show legend details toggle when layer has not legend details

* rename FillableVector to FillableRectangle

* use mixin pattern instead of encapulated function call
nreese added a commit that referenced this pull request May 7, 2019
@nreese
[Maps] show dynamic style ranges in legend (#35417) (#36229)
9801e56 
* show grab and edit buttons when hovering over layer name

* display layer details when layer name is clicked

* add range to dynamic style property state

* render dynamic ranges in TOC details

* render symbol size and border width header in legend

* simplify VectorStyle.getIcon

* removed unused component

* open TOC details on map load

* save open TOC details state in embeddable config

* show gradients for dynmaic fill color icon

* round corners of dynamic icon if points only

* add tooltip to legend label

* add edit panel action to action panel

* add functional test for details in legend

* fix broken gis_page function doesLayerExist

* add unit tests for VectorStyle.getDescriptorWithDynamicRanges

* open actions menu on layer title click, add arrow up/down for togging layer details

* Design cleanup of layers panel (#31)

* fix functional test

* update jest snapshots

* refactor StylePropertyLegendRow to use same function to render lineWidth and iconSize legends

* fix functional test

* fix another functional test

* make escapeLayerName function instead of instance method

* move _isLayerDetailsOpen into prop from redux connector

* remove index.js file

* do not show legend details toggle when layer has not legend details

* rename FillableVector to FillableRectangle

* use mixin pattern instead of encapulated function call
@nreese nreese mentioned this pull request May 8, 2019
Closed
@nreese nreese mentioned this pull request May 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasneirynck thomasneirynck thomasneirynck approved these changes

@cchaos cchaos Awaiting requested review from cchaos

@kindsun kindsun Awaiting requested review from kindsun

Assignees
No one assigned
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:enhancement review v7.2.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nreese @elasticmachine @alexfrancoeur @cchaos @kindsun @thomasneirynck