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

[Lens] Make histogram brushing possible #79435

Merged
merged 3 commits into from
Oct 7, 2020

Conversation

flash1293
Copy link
Contributor

Fixes #79260

This PR checks for histogram charts as well and allows brushing with the current context so a range filter on the number field of the x axis will be set

@flash1293 flash1293 added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 Feature:Lens labels Oct 5, 2020
@flash1293 flash1293 marked this pull request as ready for review October 5, 2020 12:59
@flash1293 flash1293 requested a review from a team October 5, 2020 12:59
@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Oct 5, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@mbondyra
Copy link
Contributor

mbondyra commented Oct 5, 2020

It works fine and I didn't find any weird usecases. It would be nice to have this 'snapToLast' functionality but it's not a blocker to merge this one.

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.

Tested in Chrome and I'm not sure that the benefits of this feature outweigh the downsides: it doesn't feel like it's polished to the level most features in Lens are. My preference is to merge a change to 7.10 to remove the onBrushEnd property from Settings unless it's a timestamp, which will remove the user expectation that they can brush on number histograms.

There are two main features that I don't think are ready yet, but that I would want before releasing this:

  1. Snap to the predefined intervals
  2. If the user has a range filter on the same field used for the X axis, we should modify the existing filter instead of adding a second filter. Otherwise multiple filters are being created as you zoom in.

x-pack/plugins/lens/public/xy_visualization/expression.tsx Outdated Show resolved Hide resolved
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.

The issues I was raising are potentially not blocking. I still think the behavior is less than ideal, but I won't block this one from merging in case we choose this solution. Changing my review from "request changes" to approving.

@flash1293
Copy link
Contributor Author

flash1293 commented Oct 6, 2020

Thanks for taking a look @wylieconlon. I thought about the issue a bit and I think we should merge the current status of this PR instead of focusing on implementing the features you mentioned right now - the reason for this is I don't think the current behavior is strictly worse than the proposed alternatives and requires much more changes.

Snap to the predefined intervals

There are some cases where the snapping behavior is not the right thing to do because it's unnecessarily taking away control from the user. As we can't know whether the user wants this level of control, IMHO we shouldn't restrict them if there isn't a good reason to do so. The only downside of the current behavior I can see is getting "odd" ranges (like 3463 to 4212), but I don't see this as a significant downside. Also it's consistent with how brushing works in a bunch of other places across Kibana. Let's discuss this separately (what about snapping to the predefined intervals if the shift key is held down? Other apps use this behavior as well). I will open an issue on elastic-charts for this.

If the user has a range filter on the same field used for the X axis, we should modify the existing filter instead of adding a second filter. Otherwise multiple filters are being created as you zoom in.

I agree this behavior is not optimal, but it's consistent with how brushing works in Visualize today. Also, I'm not sure whether overwriting an existing range filter would even be the right option for us right now because we can't use the back button to go back to the previous state - if you brush the second time, you can only go back to the initial state without filter, not to the previous intermediate one (not starting with possible interactions with pinned/saved filters which were present prior to brushing). With the behavior on this PR you can scope down the range by brushing multiple times, then go back by removing the range filters from right to left. It's not the most polished behavior, but I think getting to a really nice UX here requires more work than just modifying the existing filter.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
lens 1.1MB 1.1MB +102.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Number histogram brushing doesn't add filter
5 participants