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.

Sign up for GitHub

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

Jump to bottom

Fix visualize a field through discover app #73652

Merged
merged 3 commits into from Jul 30, 2020
Merged

Fix visualize a field through discover app #73652

merged 3 commits into from Jul 30, 2020

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Jul 29, 2020 •
edited

Summary

Fixes #73326

The fix is based on setting initial agg id as 1 instead of 2.
The previous 2 id caused a recalculation of basic seriesParams in Metrics & Axes options tab.

Result:

field_visualize

Checklist

For maintainers

@sulemanof sulemanof added v7.10.0 v7.9.1 v8.0.0 release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Discover Discover Application labels Jul 29, 2020
@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

@sulemanof sulemanof marked this pull request as ready for review July 29, 2020 13:54
@sulemanof sulemanof requested a review from a team July 29, 2020 13:54
@elasticmachine
Copy link
Contributor

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

@sulemanof
Copy link
Contributor Author

The CI is not stable today unfortunately 😢
But that's not related to my changes 💯

dej611
dej611 reviewed Jul 29, 2020
View reviewed changes
Copy link
Contributor

@dej611 dej611 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, Firefox and Safari the bug fix (before and after) and 👍 .

Could you add a little bit more context on the id choice of 1 and 2?
The file looks started with id 2 at the beginning of time: it is not clear to me this arbitrary choice.

@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

@sulemanof
Copy link
Contributor Author

Could you add a little bit more context on the id choice of 1 and 2?
The file looks started with id 2 at the beginning of time: it is not clear to me this arbitrary choice.

I don't see any reason in choosing the 2 id here. I would say this was a historical mistake 🤔
Frankly, you could set up any id , even 100500, it doesn't matter for aggregation specific.
But if we are creating the Area chart, there we have additional controls in Metrics & Axes tab, where the default settings specify the Y-axis params in accordance to the first metric aggregation, it expects the first metric agg has 1 id.
So if the default spec is not compatible with the first agg (like we had in the issue), the spec will be recalculated.

stratoula
stratoula approved these changes Jul 30, 2020
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

LGTM, tested it on safari. Bug is solved 🎉 My dear @sulemanof thanx for the test too ❤️

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@sulemanof sulemanof merged commit 6fc193c into elastic:master Jul 30, 2020
@sulemanof sulemanof deleted the fix/discover_field_visualize branch July 30, 2020 13:36
This was referenced Jul 30, 2020
Merged
Merged
sulemanof added a commit to sulemanof/kibana that referenced this pull request Jul 30, 2020
@sulemanof @elasticmachine
Fix visualize a field through discover app (elastic#73652)
d6a2295 
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
sulemanof added a commit that referenced this pull request Jul 31, 2020
@sulemanof @elasticmachine
Fix visualize a field through discover app (#73652) (#73800)
079d346 
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
sulemanof added a commit that referenced this pull request Jul 31, 2020
@sulemanof @elasticmachine
Fix visualize a field through discover app (#73652) (#73801)
6f73563 
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dej611 dej611 dej611 left review comments

@stratoula stratoula stratoula approved these changes

@timroes timroes Awaiting requested review from timroes

Assignees
No one assigned
Labels
Feature:Discover Discover Application release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.1 v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
5 participants
@sulemanof @elasticmachine @kibanamachine @dej611 @stratoula