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

fixes vislib legend filters #29592

Merged
merged 9 commits into from
Feb 14, 2019
Merged

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Jan 30, 2019

Summary

Resolves #28176
Resolves #29593

Release Note:
legend filtering for area/bar/line charts is working again

Checklist

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

For maintainers

@ppisljar ppisljar added review release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.6.0 v6.7.0 labels Jan 30, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@ppisljar ppisljar requested a review from a team as a code owner January 30, 2019 11:04
@ppisljar ppisljar changed the base branch from master to 6.x January 30, 2019 11:05
@timroes
Copy link
Contributor

timroes commented Jan 30, 2019

This should get a couple of functional tests to prevent further regressions.

@ppisljar
Copy link
Member Author

yeah, but in a separate PR, as we want those tests on master as well.

@timroes
Copy link
Contributor

timroes commented Jan 30, 2019

Indeed, but could you perhaps make sure that test PR is first merged, and this PR here is rebased so we can make sure it's working with the tests.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor

timroes commented Jan 31, 2019

Jenkins, test this - tested locally seems to work obviously I tested the wrong PR locally ..

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor

timroes commented Jan 31, 2019

Removed the code for fixing #29593, since that also needs to go to master, and this is just a fix for 6.x. So that issue will need to go into a separate PR.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@JacobBrandt
Copy link
Contributor

JacobBrandt commented Jan 31, 2019

@timroes I applied this change and noticed that the legend filter options always show up now.
legend_filter

This doesn't make sense in this case and applying this filter just picks the first thing drawn from the x-axis.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar
Copy link
Member Author

retest

@JacobBrandt
Copy link
Contributor

JacobBrandt commented Feb 12, 2019

Tested latest changes and it's still not right. Still showing legend filter for Count which doesn't make sense. Also other bucket doesn't work right. Filtering on other doesn't negate all the other terms in the legend it only negates one.

Also the filters don't even show up for multiple split series.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293
Copy link
Contributor

flash1293 commented Feb 13, 2019

@JacobBrandt

Tested latest changes and it's still not right. Still showing legend filter for Count which doesn't make sense. Also other bucket doesn't work right. Filtering on other doesn't negate all the other terms in the legend it only negates one.

These two work correctly for me (vertical bar chart):
countfilter
otherfilter

@ppisljar

Also the filters don't even show up for multiple split series.

I can reproduce this one - two "Split Series" aggs don't show the filter anymore.

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

I've tested and found the following :

  • ✅filter buttons are hidden on a single series aggregation

screenshot 2019-02-13 at 09 43 03

screenshot 2019-02-13 at 09 43 31

  • ✅filter buttons are visible after a splitted series configuration (also without an x axis configured)

screenshot 2019-02-13 at 09 44 49

screenshot 2019-02-13 at 09 54 25

  • ✅clicking + filter button on other will cause adding a filter that will shows the other buckets (NOT the non-other)

screenshot 2019-02-13 at 09 51 50

  • ✅clicking - filter button on other will cause adding a filter that will shows only non-other buckets (limited to the size of the aggregation bucket)

screenshot 2019-02-13 at 09 52 01

  • ⛔️when using multiple split series no filter buttons appear

screenshot 2019-02-13 at 10 05 53

@ppisljar
Copy link
Member Author

thanks a lot to everyone for testing this one!

@ppisljar
Copy link
Member Author

@JacobBrandt, thanks a lot for your help, i really appreciate it! Would you mind checking again the other bucket issue and filter buttons on count ? Does it only happen with some specific configuration or was it really general ?

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Tested also on multiple split series and now it works.
screenshot 2019-02-13 at 10 40 23

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@JacobBrandt
Copy link
Contributor

JacobBrandt commented Feb 13, 2019

@ppisljar I must have been looking at that Count issue wrong. I don't see that happening. Split series now shows the filters again. I still see the other bucket filtering issue I was talking about as well as the split series filtering issue I already mentioned earlier. Not sure if you want to fix these in this ticket but just wanting to let you know.

Here is other bucket legend filtering picking the first in it's series. This one is odd because other means different things in each series so I'm not sure what it should filter on. I was thinking perhaps in this case it should filter out everything visible in the legend. This would make it behave logically as far as the legend display is concerned. Right now it's just whatever the first other in the series is which is not easy to discern which one that is or that it's going to do that.

EDIT: The more I look at it feels like other shouldn't be by itself unless it's not filterable. It highlights all others but then you can't filter on that highlighted group because some other terms are not other terms in the other series. Maybe in this case It should behave more like the split series and have "Kibana - Other", "Logstash Airways - Other", etc. With an additional non filterable legend of Other. The filtering with "Kibana - Other" would request to do both (like split series should do) and in the filter bar they can choose which ones to apply.
other_bucket_filtering

Here is the split series filtering picking the first in it's series. This one should filter on both values in the legend but picks one of the terms which again is not easy for the user to understand which one it will do.
split_series_filtering

Hope that helps.

@ppisljar
Copy link
Member Author

thanks a lot for testing this @JacobBrandt

i created two issues: #31071 #31072

they are also present on master, so i will work on them separately, also they are not regressions as seems those behaved like that since initial implementation.

@ppisljar ppisljar merged commit c266134 into elastic:6.7 Feb 14, 2019
ppisljar added a commit to ppisljar/kibana that referenced this pull request Feb 14, 2019
# Conflicts:
#	test/functional/apps/visualize/_vertical_bar_chart.js
@bhavyarm
Copy link
Contributor

On BC1 of 6.6.1 - this commit is missing.

@bhavyarm
Copy link
Contributor

Please note the backport of this won't make 6.6.1. I have removed the 6.6.1 label. Thanks!

@dougburks
Copy link

I see the v6.6.2 label, but I don't see this issue listed in the 6.6.2 Release Notes:
https://www.elastic.co/guide/en/kibana/6.6/release-notes-6.6.2.html

Did the fix actually make it into the 6.6.2 release?

@timroes
Copy link
Contributor

timroes commented Mar 13, 2019

@dougburks that should have been in the 6.6.2. @gchaps do you have an idea why this PR is missing from the release notes? It looks like it should be labelled correctly?

Update: maybe it was the missing feature label. Could you please still add this to the release notes.

@timroes timroes added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Mar 13, 2019
@dougburks
Copy link

Hi @timroes and thanks for the quick response!

If this is in 6.6.2, then may I ask for some clarification? I was tracking a change in Kibana behavior that started in 6.6.0 that I thought was #28176 and to be fixed by this PR. That change in behavior still exists in 6.6.2.

Here's an example:

  • In previous versions of Kibana (up to 6.5.4), when I hover over a value in the legend, I can then click anywhere in the dark grey area to then show the magnifiers (meaning I don't have to click directly on the 7:
    Kibana-bug-6 5 4-expected

  • Starting in 6.6.0 (and including 6.6.2), I now have to click directly on the number 7 itself (otherwise the magnifiers never show):
    Kibana-bug-6 6 2

So if this PR is indeed in 6.6.2, is the behavior I'm seeing intended, or do I need to open another issue?

@gchaps
Copy link
Contributor

gchaps commented Mar 13, 2019

@timroes This issue is now included in the 6.6.2 Release Notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix review Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.6.2 v6.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants