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] Support categorical styling for numbers and dates #57908

Merged
merged 41 commits into from
Mar 9, 2020

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Feb 18, 2020

Closes #54923

Introduces a few changes:


Adds quantitative/qualitative selector.

image

image

image


It adds number/date fields to the icon-selector.


It removes all the get[Number|Date|...]Field methods from Source and Layer, in favor of letting the vector-style-editor component filter out fields by type.

@elasticmachine
Copy link
Contributor

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

@miukimiu miukimiu self-requested a review February 19, 2020 12:15
@thomasneirynck thomasneirynck marked this pull request as ready for review February 20, 2020 16:40
@thomasneirynck thomasneirynck requested a review from a team as a code owner February 20, 2020 16:40
@thomasneirynck thomasneirynck added the WIP Work in progress label Feb 20, 2020
@thomasneirynck
Copy link
Contributor Author

thomasneirynck commented Feb 20, 2020

Assigned review, wanting to get a sense if this is the right way to do it. Basically, just use the "type" field on the dynamic_style-property to toggle between categorical/qualitative.

Custom-palettes are not working yet. Still need to check what's going wrong there.

@nreese
Copy link
Contributor

nreese commented Feb 21, 2020

@elasticmachine merge upstream

@nreese
Copy link
Contributor

nreese commented Feb 21, 2020

I think this is a good approach.

Just a few comments. "Quantitative" and "Qualitative" are complex words for users without a strong math/data visualization background. I think we need some help text or tooltip to explain the difference between the two.

Should add number and date fields to icon style by value as well so numbers can be used for setting icon.

'xpack.maps.styles.dynamicColorSelect.qualitativeOrQuantitativeTooltip',
{
defaultMessage:
'Choose `quantitative` to map the value as a number to a color on a range. Choose `qualitative` to map the value as a category to a color from a palette',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gchaps I could probably use some help with some good help-text here 😃 Any ideas to make this read better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a tooltip, how about EuiFormRow help text for the selected value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do either of these work?

Choose Quantitative to map the value to a number in a color range, or Qualitative to map to a category in a color palette.

Choose Quantitative to map by number in a color range, or Qualitative to categorize by color palette.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx @gchaps !

to avoid some clutter in the UX (which is getting really long), we were thinking if we could avoid the help-text if the select-box uses "As number" and "As category" for the options. It'd still using your help-text in the aria-label.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

That wording is much clearer than using quantitative and qualitative.

But should it be "By number" and "By category"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gchaps maybe (?) :) not super familiar with the nuances here.

It's basically to express the difference in how the value should be interpreted. Should the value be interpreted as a number on a range, then we can map it to a continuous color-range. Should the value be interpreted as a category, then we just assign a color-value.

I can make this change to 'by', but 'as' seems to fit a little better by what it's trying to express.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. 'as' is the better choice.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Nice improvements. Really like removing of getDateFields, getNumberFields, and getCategoricalFields

Copy link
Contributor

@nreese nreese 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 green ci
code review, tested in chrome

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@thomasneirynck thomasneirynck merged commit 6e5e8c8 into elastic:master Mar 9, 2020
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Mar 9, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 10, 2020
* master: (22 commits)
  Generate docs from data plugin (elastic#56955)
  Fixes elastic#59513 by hiding one of the symmetric edges rather than omiting it (elastic#59514)
  Harden creation of child processes (elastic#55697)
  [Alerting] replace watcher http APIs used by index threshold Alerting (elastic#59475)
  [Maps][docs] add more details to Quantitative data driven styling docs (elastic#59553)
  chore: 🤖 hide Drilldowns in master (elastic#59698)
  [Discover] Migrate AppState/GlobalState to new app state helpers (elastic#57175)
  Use HTTP request schemas to create types, use those types in the client (elastic#59340)
  [Maps] Support categorical styling for numbers and dates (elastic#57908)
  [ML] Functional API tests - bucket span estimation with custom search.max_buckets (elastic#59665)
  Fix slm_ui setting by changing camel case back to snake case. (elastic#59663)
  removes beta tag (elastic#59618)
  [DOCS] Removed spatial references (elastic#59595)
  fix outdated docs (elastic#58729)
  [ML] Fixes bucket span estimators loading of max_buckets setting (elastic#59639)
  [ML] Refactoring anomaly detector job types (elastic#59556)
  [Upgrade Assistant] Better handling of closed indices (elastic#58890)
  additional visualizations plugin cleanup before moving to NP (elastic#59318)
  In scripted fields, unable to switch the `Type` - getting a console error which says - Class constructor DecoratedFieldFormat cannot be invoked without 'new' (elastic#59285)
  [Visualize] Remove global state in visualize (elastic#58352)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 10, 2020
…s/kibana into alerting/fix-flaky-instance-test

* 'alerting/fix-flaky-instance-test' of github.com:gmmorris/kibana: (176 commits)
  Generate docs from data plugin (elastic#56955)
  Fixes elastic#59513 by hiding one of the symmetric edges rather than omiting it (elastic#59514)
  Harden creation of child processes (elastic#55697)
  [Alerting] replace watcher http APIs used by index threshold Alerting (elastic#59475)
  [Maps][docs] add more details to Quantitative data driven styling docs (elastic#59553)
  chore: 🤖 hide Drilldowns in master (elastic#59698)
  [Discover] Migrate AppState/GlobalState to new app state helpers (elastic#57175)
  Use HTTP request schemas to create types, use those types in the client (elastic#59340)
  [Maps] Support categorical styling for numbers and dates (elastic#57908)
  [ML] Functional API tests - bucket span estimation with custom search.max_buckets (elastic#59665)
  Fix slm_ui setting by changing camel case back to snake case. (elastic#59663)
  removes beta tag (elastic#59618)
  [DOCS] Removed spatial references (elastic#59595)
  fix outdated docs (elastic#58729)
  [ML] Fixes bucket span estimators loading of max_buckets setting (elastic#59639)
  [ML] Refactoring anomaly detector job types (elastic#59556)
  [Upgrade Assistant] Better handling of closed indices (elastic#58890)
  additional visualizations plugin cleanup before moving to NP (elastic#59318)
  In scripted fields, unable to switch the `Type` - getting a console error which says - Class constructor DecoratedFieldFormat cannot be invoked without 'new' (elastic#59285)
  [Visualize] Remove global state in visualize (elastic#58352)
  ...
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] Support color palettes for number and date values
7 participants