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

Add AggParamFilters to filter out params in vis editor #20686

Closed
wants to merge 3 commits into from

Conversation

jen-huang
Copy link
Contributor

Similar to #19913 and #20539, this adds an additional AggParamFilters registry to filter out params for an aggregation type.

This is need to support rollups (#20004), notably to filter out missingBucket and otherBucket params.

Also includes a fix for if the default selected aggregation type is not part of the filtered agg type list, switch selection to the first available agg type: b9e6fe8

@jen-huang jen-huang added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.0.0 v6.4.0 labels Jul 11, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor

timroes commented Jul 12, 2018

I think we had some kind of miscommunication here :-) I thought that hiding parameters should be part of the EditorConfig in the end and not get it's own filter.

My reasoning behind that would be: aggregations and fields are kind of very special for all aggregations so I think they deserve their own filters (and for fields I already wondered if we shouldn't put that into EditorConfig but decided to have it separated). I think if we now start to create individual registries for everything we need to do with parameters (hiding, setting to fixed values, using a base), that will cause us to end up with a lot of super small registries what I would like to avoid. Also If we separate all those different "parameter configurations" we can't properly validate them against each other anymore (e.g. via proper typing). For example it doesn't make much sense to specify a base for a field and hide it, or later show a limitation warning (which would be part of the config) and hide the field.

If we put all of that in the config (like https://github.com/elastic/kibana/pull/20519/files#diff-ead2d626d94f97e12d549312159afedbR22) we can actually properly validate those and would only end up with one additional registry and not 3-5 more of them. This would also potentially make it easier to take over some of this code for the new editor (since with the current early stage we're having for the new editor, I am not sure if not all of those is throw away code after all).

Sorry that I didn't convey that idea properly in the beginning. What are your thoughts around that, since we can of course discuss all of that.

@jen-huang
Copy link
Contributor Author

@timroes For some reason I was under the impression that EditorConfig was just for modifying other parameter configurations, I didn't realize it could also be used to toggle visibility 😄 I agree, it would be better to avoid micro-registries. Closing this!

@jen-huang jen-huang closed this Jul 12, 2018
@jen-huang jen-huang deleted the aggparams-filter branch July 12, 2018 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Rollups Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v6.4.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants