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

fix(goal): reduce whitespace for circular charts #1413

Merged
merged 1 commit into from
Oct 5, 2021

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Sep 28, 2021

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.

Screen Recording 2021-09-28 at 01 06 10 PM

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

  1. The user can set angleStart and angleEnd to any value thus allowing for nonsensical angle values. Such values are not accounted for in this approach.
  2. The chart allows but does not account for a full circle goal chart and assumes the extents of the angle does not collide with the bottom titles. This also assumes the center of the full circle is at the center of the chart, hence why full circles collide with the bottom titles. See gif showing this below.
  3. The central titles/labels are assumed to be at the center of the circular chart, thus shifting beyond half the radius is not practical until more complex center placement is allowed.
  4. The size of the chart is computed based on hard-coded limiting sizes. This is ideal for now as any translation and resizing would require some iteration to optimize the size given the value of angleStart and angleEnd.

Screen Recording 2021-09-28 at 01 07 45 PM

Solution

The solution uses the angleStart and angleEnd to 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π/2 or -π/2). Let's call these θL (aka theta-L) and θR for 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.

Untitled

From here we can now determine the so called sagitta (S in the diagram below) from θ and the outer radius of the goal chart.

Untitled 2

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π/2 or θmax. This was chosen to avoid the title, see limitation 2.

Untitled 3

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 |

Untitled 4

Notice the Smin on the far left chart is limited to half the radius as mentioned above.

Issues

closes #1240

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

@nickofthyme nickofthyme added enhancement New feature or request :goal/gauge (old) Old Goal/Gauge chart related issues labels Sep 28, 2021
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

Looks great 🎉

This single story needs an update, shorter text or different angles:
image—unless the purpose of the story is to show that overlap can and does happen, depending on the config

The geometry driven solution is nicer than some ad hoc thing would have been, good call 💪

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;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

How is the sometimes considerable stroke width of the arc taken into account?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

@nickofthyme
Copy link
Collaborator Author

This single story needs an update, shorter text or different angles:

Yeah I want to make sure the beyond 5π/4 and -π/4 there is no offset applied since this is the current max value.

The geometry driven solution is nicer than some ad hoc thing would have been, good call 💪

🙇🏼

@nickofthyme nickofthyme merged commit 6517523 into elastic:master Oct 5, 2021
@nickofthyme nickofthyme deleted the goal-spacing branch October 5, 2021 15:21
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)
@nickofthyme
Copy link
Collaborator Author

🎉 This PR is included in version 38.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request :goal/gauge (old) Old Goal/Gauge chart related issues released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce wasted space in angular charts
2 participants