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

[Vis: Default editor] EUIficate Percentile Ranks and Percentiles #35349

Merged
merged 36 commits into from
May 29, 2019

Conversation

maryia-lapata
Copy link
Contributor

@maryia-lapata maryia-lapata commented Apr 19, 2019

Summary

EUIfication of Percentile Ranks and Percentiles controls for aggregation parameter in Default Editor, Data tab.
Part of #30922.

The old Percentile Ranks and Percentiles controls used number-list directive, so the directive was EUIficated to NumberList component as well.

image

Steps to reproduce: Create Area visualization, expand Metrics group, choose Percentile Ranks or Percentiles aggregation.

Details

  1. In old version it was allowed an empty value, but under the hood the 0 value was sent to the model instead of undefined or empty string. I think it's not clearly for a user, so I made a value field is required: the user has to enter a value or remove an empty field. (cc/ @cchaos @timroes )

  2. If values are not specified, a value with 0 is displayed by default.

image

  1. If there is the only value, the remove button is disabled.

  2. If entered value is not within the defined range, an error message will be displayed under that value field.

image

  1. If the values should be in ascending order (as it's by default, e.g. for Percentile Ranks) and if they are not in such order, an error message will be displayed under the all values

image

  1. The old number-list directive supported several additional keyboard shortcuts: shift-up/shift-down increases/decreases by 0.1, shift-enter adds a field, backspace and delete allow to remove a field when it's empty. I didn't implement them in NumberList component because
    a) there is no any mention about such keyboard possibility (I don't think that users used them),
    b) such hot keys support should be more common, probably Percentile Ranks and Percentiles in TSVB should also have the same keyboard shortcuts, or even EuiFieldNumber might support shift-up/shift-down shortcuts.

This PR also contains removal of the directives that are not used anymore:

  • kbnNumberList (src/legacy/ui/public/agg_types/number_list/number_list.js)
  • kbnNumberListInput (src/legacy/ui/public/agg_types/number_list/number_list_input.js)

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@maryia-lapata maryia-lapata requested a review from sulemanof April 19, 2019 12:54
@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@maryia-lapata maryia-lapata requested a review from flash1293 May 22, 2019 07:32
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Ok, we can delay no. 1 & 2 for now, but should try to solve them before the whole EUI-ification of Visualize is done.

Thanks, LGTM!

@timroes
Copy link
Contributor

timroes commented May 24, 2019

With regards to 1. @cchaos I am not so sure if we should fix that for the old editor. The problem here is that a field is just a regular parameter to the aggregation the same as any other parameter. Currently when switching aggregations we'll reset all parameters, since they might not make sense on the new aggregation anymore or even worse: there might be a parameter with the same name, but expecting different input (e.g. the interval on the different histogram aggregations). If we want to keep the field (and we have the same issue there, the field valid for one aggregation might not be valid for another), we would need to build some very special handling for that parameter, that by itself is not really different (from a technical point of view) than any other parameter. So basically we would look for a parameter named field trying to keep it's value, but basically manually must trigger validation for it, since we don't know if the same field is still valid, and if we can reuse manually trigger the change behavior for the field, so the aggregation is setup correctly. This sounds like some unpleasant and hard to maintain code in the end, which I am not sure if we want to introduce it. I totally agree that it would be better usability, but the old editor was just not build that way, and I am not sure if we should hack that special behavior still into it, or rather wait for the new visual editor to solve those general usability issues (don't select aggregation before field) for the users.

@elasticmachine

This comment has been minimized.

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 haven't done a really thorough code review, but checked out locally and the functionality is basically working. I had some issues using Percentile Ranks but I think that might be user error.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@maryia-lapata maryia-lapata requested a review from lukeelmers May 28, 2019 06:50
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.

LGTM, compared against functionality on master

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@maryia-lapata maryia-lapata merged commit 1a898b1 into elastic:master May 29, 2019
@maryia-lapata maryia-lapata deleted the number-list branch May 29, 2019 11:32
maryia-lapata added a commit to maryia-lapata/kibana that referenced this pull request May 29, 2019
jkakavas pushed a commit to jkakavas/kibana that referenced this pull request May 30, 2019
…stic#35349)

* Migrate number-list

* Add types

* Refactoring

* Add number row

* Add and remove items

* Add onChange

* Add validate function

* Apply validation

* Fix merge conflict

* Apply order validation

* Fix ts

* Refactoring

* EUIficate percentiles control

* Remove unused translations

* Move helper functions to a separate file

* Fix code review comments

* Make add button 'xs' size

* Remove extra space

* Fix validation

* Remove unused translations
@stacey-gammon stacey-gammon mentioned this pull request Jun 30, 2019
94 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:Vis Editor Visualization editor issues release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants