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

removes :filters from label #12962

Merged
merged 2 commits into from
Aug 1, 2017
Merged

Conversation

ppisljar
Copy link
Member

closes #12827

@ppisljar ppisljar added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v6.0.0 labels Jul 19, 2017
@nreese
Copy link
Contributor

nreese commented Jul 19, 2017

Took me a little while to reproduce the problem. Split table with filters agg to reproduce.
screen shot 2017-07-19 at 12 27 26 pm

@@ -75,7 +75,10 @@ export function TabbedAggResponseWriterProvider(Private) {
if (group) {
table.aggConfig = agg;
table.key = key;
table.title = (table.fieldFormatter()(key)) + ': ' + agg.makeLabel();
table.title = (table.fieldFormatter()(key));
if (agg.makeLabel() !== 'filters') {
Copy link
Contributor

@nreese nreese Jul 19, 2017

Choose a reason for hiding this comment

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

This problem really affects any aggregation that does not implement makeLabel function and relies on the default implementation (which just returns the aggregation name). How about instead of hard coding to check against filters you check that agg.makeLabel() !== agg.name? A comment about why this check is needed would also be helpful.

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

@ppisljar
Copy link
Member Author

jenkins, test this

@ppisljar ppisljar force-pushed the fix/filterSplitLabel branch from 6f1c22a to ca5e892 Compare July 27, 2017 08:22
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.

lgtm, but I'd recommend moving this more clear by moving the implementation of the evaluation to aggConfig. It will make code more readable and navigable.

table.title = (table.fieldFormatter()(key)) + ': ' + agg.makeLabel();
table.title = (table.fieldFormatter()(key));
// aggs that don't implement makeLabel should not add to title
if (agg.makeLabel() !== agg.name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this a method on the aggConfig. Something like isAggConfigOverridden, along those lines.

@ppisljar ppisljar merged commit baabbdf into elastic:master Aug 1, 2017
ppisljar added a commit to ppisljar/kibana that referenced this pull request Aug 1, 2017
ppisljar added a commit to ppisljar/kibana that referenced this pull request Aug 1, 2017
ppisljar added a commit that referenced this pull request Aug 1, 2017
ppisljar added a commit that referenced this pull request Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v6.0.0-rc1 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleting ": filters" in the Data table
4 participants