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(goal): chart placement and overlap issues #1620

Merged
merged 10 commits into from
Mar 14, 2022
Merged

fix(goal): chart placement and overlap issues #1620

merged 10 commits into from
Mar 14, 2022

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Mar 4, 2022 •
edited
Loading

Summary

This PR fixes several issues related to goal/gauge charts:

Corrects placement of goal charts #1598 (2fa9897)

Screen.Recording.2022-03-04.at.03.13.46.PM.mp4

Prevent overlapping angles (3982d31)

We now limit the angle provided by the user to prevent overlapping gauge/goal charts. This limit of 2π is taken from the angleStart as the baseline accounting for the original angular direction (i.e. clockwise). A warning will print out below to inform the user the value has been replaced.

[@elastic/charts] The total angle of the goal chart must not exceed 2π radians.To prevent overlapping, the value of `angleEnd` will be replaced.

  original: -1.9634954084936207 (-0.625π)
  replaced: -1.5707963267948966 (-0.5π)
Screen.Recording.2022-03-04.at.04.13.14.PM.mp4

Details

The sagitta offset calculations did not account for inverted angles nor a gap at the top of the chart. This PR fixes these cases however this is a tedious solution that requires case-by-case logic just to improve the positioning of the chart and is limited to what angles can be used (i.e. only applicable to angles where -2π > θ > 2π with the smallest angle from 0).

A better solution would be to update a rectangular box that contains all geoms of the goal/gauge content, including text labels, then using the centroid of the rectangle to correctly place the chart inside the available area.

test

Here 𝝙x and 𝝙y are the optimal offset given the blue rectangle containing all chart geoms.

Issues

fix #1598

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • 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

@nickofthyme nickofthyme added :chart Chart element related issue :goal/gauge (old) Old Goal/Gauge chart related issues labels Mar 4, 2022
@nickofthyme nickofthyme changed the title fix: goal chart placement, overlap and sizing issues fix(goal): chart placement, overlap and sizing issues Mar 4, 2022
@nickofthyme nickofthyme marked this pull request as ready for review March 4, 2022 22:49
markov00
markov00 approved these changes Mar 10, 2022
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

It looks good to me now, tested on hosted storybook!

packages/charts/src/chart_types/goal_chart/layout/viewmodel/utils.test.ts Show resolved Hide resolved
markov00
markov00 reviewed Mar 10, 2022
View reviewed changes
.eslintrc.js Show resolved Hide resolved
@nickofthyme nickofthyme merged commit b5d375b into elastic:master Mar 14, 2022
@nickofthyme nickofthyme deleted the fix-goal-placement branch March 14, 2022 14:46
@nickofthyme nickofthyme changed the title fix(goal): chart placement, overlap and sizing issues fix(goal): chart placement and overlap issues Mar 14, 2022
nickofthyme pushed a commit that referenced this pull request Mar 29, 2022
@semantic-release-bot
chore(release): 45.1.0 [skip ci]
ee12240 
# [45.1.0](v45.0.1...v45.1.0) (2022-03-29)

### Bug Fixes

* **axis:** ordinal number ending fix for the weekly resolution ([#1634](#1634)) ([18b4077](18b4077))
* **deps:** update dependency @elastic/eui to ^52.2.0 ([#1632](#1632)) ([7e0be07](7e0be07))
* **deps:** update dependency @elastic/eui to v50 ([#1622](#1622)) ([0eb7975](0eb7975))
* **deps:** update dependency @elastic/eui to v51 ([#1624](#1624)) ([64d87e5](64d87e5))
* **deps:** update dependency @elastic/eui to v52 ([#1630](#1630)) ([ada254e](ada254e))
* **goal:** chart placement and overlap issues ([#1620](#1620)) ([b5d375b](b5d375b))

### Features

* **goal:** expose max sizing limits in theme.goal options ([#1621](#1621)) ([60a14ba](60a14ba))
@markov00 markov00 mentioned this pull request Mar 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markov00 markov00 markov00 approved these changes

@monfera monfera Awaiting requested review from monfera

Assignees
No one assigned
Labels
:chart Chart element related issue :goal/gauge (old) Old Goal/Gauge chart related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Goal] Full circle is cut off when container is resized
2 participants
@nickofthyme @markov00