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

feat: add back minBarHeight on discover histogram #168644

Merged
merged 12 commits into from
Dec 5, 2023

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Oct 11, 2023

Summary

Adds back min bar height to discover for large document count variances per #68436

Details

The option for minBarHeight was likely removed from the discover histogram in process of migrating to Lens embeddable.

This PR adds a new minBarHeight option to the Lens embeddable args, this option is passed to all BarSeries in the DataLayer. This option optional and defaults to 1px.

Finally this value of minBarHeight is set to 2px for the unified histogram chart.

Before

Discover - Elastic 2023-10-11 at 9 55 59 AM

After

Discover - Elastic 2023-10-11 at 9 54 50 AM

Checklist

  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list

@nickofthyme nickofthyme requested a review from stratoula October 11, 2023 17:06
@nickofthyme nickofthyme requested review from a team as code owners October 11, 2023 17:06
@nickofthyme
Copy link
Contributor Author

If this feature is no longer desirable, I'm happy to close this PR.

@nickofthyme nickofthyme added the release_note:skip Skip the PR/issue when compiling release notes label Oct 11, 2023
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Unified Histogram changes LGTM, thanks!

@stratoula
Copy link
Contributor

stratoula commented Oct 12, 2023

@nickofthyme thanx, I think we should def do this at least for Discover. Let's not merge it until Monday that we have our weekly sync. If we decide to add it as a setting to Lens then we don't need the overrides, we will just pass it as a property to the embeddable state.

@stratoula
Copy link
Contributor

stratoula commented Oct 16, 2023

We decided to add this in Lens by default, only 1px though. We can make the changes in this PR or another. @nickofthyme whatever you want!

Copy link
Contributor Author

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

@stratoula Could you please take a look at these changes and lmk if I'm on the right track? The lens embeddable logic is still very strange to me. Thanks!

@stratoula
Copy link
Contributor

@nickofthyme you are definitely on the right track. You need to add this both in chart_expressions and in Lens. Also does it make sense to add it as an option in the toolbar? I don't remember what we agreed tbh.

image

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
expressionXY 166 167 +1
lens 531 533 +2
visualizations 804 805 +1
total +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
expressionXY 131.4KB 132.6KB +1.1KB
lens 1.4MB 1.4MB +207.0B
unifiedHistogram 51.0KB 51.1KB +15.0B
total +1.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
expressionXY 38.7KB 38.9KB +216.0B
Unknown metric groups

API count

id before after diff
expressionXY 176 177 +1
lens 630 632 +2
visualizations 835 836 +1
total +4

History

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

cc @nickofthyme

@nickofthyme
Copy link
Contributor Author

Pinging @elastic/kibana-visualizations for final review 🙏🏼

@stratoula stratoula added Feature:Lens v8.12.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure backport:skip This commit does not require backporting labels Dec 5, 2023
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.

Changes LGTM!

@nickofthyme nickofthyme merged commit 2af7030 into elastic:main Dec 5, 2023
42 checks passed
@nickofthyme nickofthyme deleted the discover-min-height-2 branch December 5, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants