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] Label style properties #52957

Merged
merged 25 commits into from
Dec 19, 2019
Merged

[Maps] Label style properties #52957

merged 25 commits into from
Dec 19, 2019

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Dec 12, 2019

fixes #52607

Adds label, labelSize, and labelColor style properties to allow users to display a label for point features.

Screen Shot 2019-12-12 at 3 06 55 PM

@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.

I love this feature! I found one minor issue.

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-docs (Team:Docs)

@nreese nreese requested a review from gchaps December 13, 2019 22:52
@jsanz
Copy link
Member

jsanz commented Dec 16, 2019

Beautiful job @nreese, having not only content but also text color and size driven by data is super cool!! 😍

Not totally sure, but it's quite typical for users to also want to change the font. Since the most common scenario is to use Maps with EMS basemaps this would not be a high priority to me at this moment since most of the visualization is already "defined" in cartographic terms, and a good default font already used in the basemap should be enough. But with custom basemaps also available, other fonts would be better suited, so we may need also that setting to be offered.

@nreese
Copy link
Contributor Author

nreese commented Dec 16, 2019

Not totally sure, but it's quite typical for users to also want to change the font

I created #53127 to track that feature request

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.

holy smokes. killer feature.

Some initial comments. Will look more tomorrow.

I ran into two little bugs with styling with icons.

  1. saving removes icon

Steps:

  • style a point layer with an icon
  • add a label
  • save the map
  • refresh the browser
    -> label is showing but icon is not showing
  1. labels are drawn under icons

Steps:

  • style a point layer with an icon
  • add a label
    -> not that label is drawn under the icon.

(fwiw, there are bugs in mapbox wrt ordering of layers. e.g. heatmap layers sometimes behave oddly when reordering. I'm not sure if this is a similar issue with mb. It might also be us adding the symbol-layer on top of the text-layer).

@thomasneirynck
Copy link
Contributor

wrt @jsanz comment and custom fonts.

Agreed on creating separate feature request. It's somewhat of a bigger issue than just offering that here. By design, font-libraries not configurable in Maps. Right now, the available fonts are a "blackboxed" hardcoded configuration specifically for EMS basemaps ((https://github.com/elastic/kibana/blob/8e9a8a84dccfa7965ce8a22362885e6cdef8b51f/src/legacy/server/config/schema.js)), and these cannot be modified (due to limitations in mapbox-gl API with registering font-libraries at runtime). I think we should punt on custom fonts until Maps supports some sort of configurable font-library and/or preset lists of guaranteed-to-exist fonts, and untie this from EMS.

@nreese
Copy link
Contributor Author

nreese commented Dec 17, 2019

I ran into two little bugs with styling with icons.

@thomasneirynck I have resolved the bugs you spotted. The icons where not displaying after saving because mapbox appears to not draw the symbol layer under another symbol layer at the same location. I resolved this by just using a single symbol layer when points are symbolized as icons.

@jsanz
Copy link
Member

jsanz commented Dec 17, 2019

@nreese I'm getting an error when trying to enable labels

Peek 2019-12-17 17-40

No errors on the network tab, sharing console screenshot:

image

Happening in both documents and aggregation layers, tested on Chromium and Firefox.

Let me know if you need further details.

@nreese
Copy link
Contributor Author

nreese commented Dec 17, 2019

I'm getting an error when trying to enable labels

@jsanz Thanks for finding this. Its all fixed now

@jsanz
Copy link
Member

jsanz commented Dec 17, 2019

Working nicely on both Chromium and Firefox, apart from the decimals topic mentioned earlier I haven't find any other relevant issue 🎉

Maybe we can put this in a separate feature request but it would be great to allow labels to optionally have a halo (color and width). This is a very common setting to improve readability, specially when points have a color ramp, but also in many other situations. Sharing same data on Kibana and QGIS.

image

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.

mostly minor comments below.

@nreese nreese requested a review from a team as a code owner December 19, 2019 18:31
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.

💣 💯 Will really empower users

lgtm on green.

agreed with @jsanz that addition of halos would help with readability, but that can always be done later.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

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! love this feature.

tested in Chrome.

@nreese nreese merged commit 8969cdc into elastic:master Dec 19, 2019
nreese added a commit to nreese/kibana that referenced this pull request Dec 19, 2019
* text styling

* label style editor UI

* wire up styles to mb

* allow string values

* remove console.log

* default getFields to provide ordinal fields for vector source

* fix vector_style jest test

* add label styles to docs

* fix prettier errors

* use index-pattern field formatter to format label

* rename LABEL to LABEL_TEXT

* review feedback

* fix problem with icons not displaying with labels

* fix functional tests

* fix canno read name of null error

* update jest expect

* fix eslint errors

* do not display label text in legend

* always show all label styling properties in editor

* review feedback
nreese added a commit that referenced this pull request Dec 19, 2019
* text styling

* label style editor UI

* wire up styles to mb

* allow string values

* remove console.log

* default getFields to provide ordinal fields for vector source

* fix vector_style jest test

* add label styles to docs

* fix prettier errors

* use index-pattern field formatter to format label

* rename LABEL to LABEL_TEXT

* review feedback

* fix problem with icons not displaying with labels

* fix functional tests

* fix canno read name of null error

* update jest expect

* fix eslint errors

* do not display label text in legend

* always show all label styling properties in editor

* review feedback
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.

[Maps] Allow features on a map to be text, as well as circles or icons
5 participants