fix(goal): reduce whitespace for circular charts#1413
Merged
nickofthyme merged 1 commit intoelastic:masterfrom Oct 5, 2021
Merged
fix(goal): reduce whitespace for circular charts#1413nickofthyme merged 1 commit intoelastic:masterfrom
nickofthyme merged 1 commit intoelastic:masterfrom
Conversation
monfera
approved these changes
Sep 29, 2021
monfera
reviewed
Sep 29, 2021
Comment on lines
+396
to
+401
| if (circular) { | ||
| const sagitta = getMinSagitta(angleStart, angleEnd, r); | ||
| const maxSagitta = getSagitta((3 / 2) * Math.PI, r); | ||
| data.yOffset.value = sagitta >= maxSagitta ? 0 : (maxSagitta - sagitta) / 2; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
How is the sometimes considerable stroke width of the arc taken into account?
Collaborator
Author
There was a problem hiding this comment.
Yeah that's true. How is the stroke width able to be set? Also I assume you are referring to the width of the colored bands themselves right?
Collaborator
Author
Yeah I want to make sure the beyond
🙇🏼 |
nickofthyme
pushed a commit
that referenced
this pull request
Oct 15, 2021
# [38.0.0](v37.0.0...v38.0.0) (2021-10-15) ### Bug Fixes * **deps:** update dependency @elastic/eui to v39 ([#1422](#1422)) ([2ee97aa](2ee97aa)) * **goal:** reduce whitespace for circular charts ([#1413](#1413)) ([6517523](6517523)) * **interactions:** change allowBrushingLastHistogramBin to true ([#1396](#1396)) ([9fa9783](9fa9783)) * **xy:** remove wrongly represented null/missing values in tooltip ([#1415](#1415)) ([e5963a3](e5963a3)), closes [#1414](#1414) ### Code Refactoring * scales ([#1410](#1410)) ([a53a2ba](a53a2ba)) ### Features * **scales:** add `LinearBinary` scale type ([#1389](#1389)) ([9f2e427](9f2e427)) * **xy:** adaptive tick raster ([#1420](#1420)) ([200577b](200577b)) * **xy:** apply the data value formatter to data values over bars ([#1419](#1419)) ([e673fc7](e673fc7)) ### BREAKING CHANGES * **interactions:** allowBrushingLastHistogramBucket renamed to allowBrushingLastHistogramBin on the Settings component defaults true and is only applied for histogram type charts * LogScaleOptions.logBase` is now a `number` instead of the object enum `LogBase`. Some edge case data or configuration _might_, with a deemed low likelihood, lead to a situation where the earlier version would have silently not rendered a bar, line or point, while the new code doesn't `catch`, therefore throw an exception (see the last item). General risk of regressions due to the quantity of code changes (altogether 3.5k)
Collaborator
Author
|
🎉 This PR is included in version 38.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
The whitespace for circular goal charts had been reduced by offsetting the chart center whenever possible. This does not increase the size of the chart but reduces whitespace between the provided title add the chart itself.
Details
This is a first step approach to reduce the whitespace only by translating the chart when possible. There are a few limitations of the current goal chart that limit other solutions as well as this one. Namely:
Limitations
angleStartandangleEndto any value thus allowing for nonsensical angle values. Such values are not accounted for in this approach.angleStartandangleEnd.Solution
The solution uses the
angleStartandangleEndto compute a minimum limiting sagitta with which we compare with the minimum limiting sagitta, assigning the offset as the delta. WHAT!??!?!We measure the angle from the top (
π/2) towards the bottom (3π/2or-π/2). Let's call theseθL(aka theta-L) andθRfor left and Right. Thenθis the limiting angle determined by the max of left and right, where theta is greater thanπ/2, see limitation 3.From here we can now determine the so called sagitta (
Sin the diagram below) fromθand the outer radius of the goal chart.Once we have the sagitta for the current angles we need to find the limiting max sagitta. This is the max sagitta such that there would be no translation. In the current implementation this an angle of
3π/2orθmax. This was chosen to avoid the title, see limitation 2.With these two sagitta values, we can take the difference (aka the whitespace between the chart and the titles) and divide by 2 in order to center the goal chart negating the white space between the chart and titles.
Left - Unshifted | Center - Max/Default | Right - Shifted |
Issues
closes #1240
Checklist
:xy,:partition)closes #123,fixes #123)dark,light,eui-dark&eui-light