-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this 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!
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 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this 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.
💚 Build Succeeded |
There was a problem hiding this 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
💚 Build Succeeded |
💚 Build Succeeded |
…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
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 toNumberList
component as well.Steps to reproduce: Create
Area
visualization, expandMetrics
group, choosePercentile Ranks
orPercentiles
aggregation.Details
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 )If values are not specified, a value with
0
is displayed by default.If there is the only value, the remove button is disabled.
If entered value is not within the defined range, an error message will be displayed under that value field.
Percentile Ranks
) and if they are not in such order, an error message will be displayed under the all valuesnumber-list
directive supported several additional keyboard shortcuts:shift-up
/shift-down
increases/decreases by 0.1,shift-enter
adds a field,backspace
anddelete
allow to remove a field when it's empty. I didn't implement them inNumberList
component becausea) 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 supportshift-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
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Documentation was added for features that require explanation or tutorials[ ] Unit or functional tests were updated or added to match the most common scenariosFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately[ ] This includes a feature addition or change that requires a release note and was labeled appropriately