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: gauge, goal and bullet graph (alpha) #614

Merged
merged 21 commits into from
Apr 14, 2020
Merged

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Mar 29, 2020

Summary

Alpha version for goal, gauge and bullet graph (original is designed by Stephen Few). It can already generate these:

image

The horizontal and bullet graph layouts increase readability and save space, without losing any of the information. It's also become broadly adopted (SAS, Excel, Tableau, DataWrapper, Qlik, ...), being a baseline to improvement attempts and more standardized than curvy gauge/goal charts that vary a lot more (with or without skeuomorphism), decreasing their readability. A small multiple of bullet graphs has superior comparability, and they're conducive to customization.

As alpha, it still has limitations, addressed in this PR or subsequent small PRs:

  • limited configurability, therefore
    • no dark mode support yet
    • non-configurable placement of bottom and central labels
    • tick style, tick label placement, target/actual/band geometry thickness, fonts, and most colors can't be configured yet
    • not all spec props admit functions yet, eg. ticks needs to be a list of numbers
    • no number / tick label formatters yet
  • there's no text fitting or truncation logic, so texts may overlap with anything
  • to avoid merge conflicts, this PR doesn't currently DRY up common constants, utils
  • render code to be simplified / DRYed
  • simplified tooltip; sometimes the "target" symbol is used for "actual", may need renaming
  • API might go through breaking change

Closes #520

Checklist

Delete any items that are not applicable to this PR.

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • [ ] This was checked for cross-browser compatibility, including a check against IE11
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • [ ] Unit tests were updated or added to match the most common scenarios

@codecov-io
Copy link

codecov-io commented Mar 30, 2020

Codecov Report

Merging #614 into master will decrease coverage by 3.00%.
The diff coverage is 29.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #614      +/-   ##
==========================================
- Coverage   70.79%   67.78%   -3.01%     
==========================================
  Files         220      236      +16     
  Lines        6409     6914     +505     
  Branches     1224     1312      +88     
==========================================
+ Hits         4537     4687     +150     
- Misses       1853     2208     +355     
  Partials       19       19              
Impacted Files Coverage Δ
src/chart_types/index.ts 100.00% <ø> (ø)
...types/partition_chart/layout/types/config_types.ts 100.00% <ø> (ø)
...es/partition_chart/layout/types/viewmodel_types.ts 80.00% <ø> (ø)
...rtition_chart/layout/viewmodel/link_text_layout.ts 7.31% <0.00%> (-0.80%) ⬇️
...ypes/partition_chart/layout/viewmodel/viewmodel.ts 13.28% <0.00%> (-0.22%) ⬇️
src/chart_types/xy_chart/specs/line_annotation.tsx 79.48% <ø> (ø)
src/utils/commons.ts 100.00% <ø> (ø)
...art_types/goal_chart/layout/viewmodel/viewmodel.ts 3.12% <3.12%> (ø)
...artition_chart/renderer/canvas/canvas_renderers.ts 6.29% <3.22%> (-0.72%) ⬇️
...pes/goal_chart/renderer/canvas/canvas_renderers.ts 9.58% <9.58%> (ø)
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 699783e...499b696. Read the comment docs.

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.

I've left few comments that should be reviewed before merging.
Seems a very clean implementation and it's interesting the landmark approach to compute position of elements. Although the approach is very clean, it seems that we are moving the computation of geometries inside the rendering phase, making it a bit more difficult to test if not just through canvas snapshots.

Comment on lines 31 to 32
export { example as spartanGoal } from './5_spartan';
export { example as spartanHorizontal } from './6_spartan_horizontal';
Copy link
Member

Choose a reason for hiding this comment

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

maybe minimal is more appropriate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 80 to 81
const circular = subtype === 'goal';
const vertical = subtype === 'verticalBullet';
Copy link
Member

Choose a reason for hiding this comment

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

These should use the GoalSubType enum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

77b9f48

I did it with the current array but that uses numeric indexing, is it a positive with $Values that its input is an object ie. referencing is more readable? (No need to look up what the heck [2] means)

@monfera
Copy link
Contributor Author

monfera commented Apr 3, 2020

Deferred feedback items so far:
#614 (comment)
#614 (comment)
#614 (comment)
#614 (comment)
Move calcs from render file to viewModel
Activate hover only if pointer is inside, or rather close to the circle

@elastic elastic deleted a comment from monfera Apr 6, 2020
@elastic elastic deleted a comment from monfera Apr 6, 2020
@markov00 markov00 changed the title feat: gauge, goal and bullet graph feat: gauge, goal and bullet graph (alpha) Apr 6, 2020
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.

All looks good to me, I've added a few minor comments that can be addressed in a later stage.
Since this an @alpha version I think it's fine to keep like that the GoalSubType but I kindly suggest to change that to be easier used as an Enumerator more than an array of values

import { TAU } from '../../../partition_chart/layout/utils/math';
import { configMap } from '../../../partition_chart/layout/config/config';

export const configMetadata = {
Copy link
Member

Choose a reason for hiding this comment

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

nit: We can avoid exporting this, it is not used anywhere except at the end of this file

import { GoalSpec } from '../../specs/index';

/** @internal */
export function shapeViewModel(textMeasure: TextMeasure, spec: GoalSpec, config: Config): ShapeViewModel {
Copy link
Member

Choose a reason for hiding this comment

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

nit: textMeasure is not used in this function, we can remove it completely

@@ -39,7 +39,8 @@ export {
FillLabelConfig as PartitionFillLabel,
PartitionLayout,
} from './chart_types/partition_chart/layout/types/config_types';
export { Layer as PartitionLayer } from './chart_types/partition_chart/specs/index';
export { Partition, Layer as PartitionLayer } from './chart_types/partition_chart/specs/index';
export { Goal } from './chart_types/goal_chart/specs/index';
Copy link
Member

Choose a reason for hiding this comment

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

nit: do you still need help on that?

labelMinor="(thousand USD) "
centralMajor="280"
centralMinor="target: 260"
config={config}
Copy link
Member

Choose a reason for hiding this comment

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

nit: if the config is the default one, we should avoid showing it on the story.

@monfera monfera merged commit 5669178 into elastic:master Apr 14, 2020
markov00 pushed a commit that referenced this pull request Apr 15, 2020
# [18.3.0](v18.2.2...v18.3.0) (2020-04-15)

### Bug Fixes

* remove series with undefined splitSeriesAccessor values ([#627](#627)) ([59f0f6e](59f0f6e))

### Features

* gauge, goal and bullet graph (alpha) ([#614](#614)) ([5669178](5669178))
* **partition:** add legend and highlighters ([#616](#616)) ([6a4247e](6a4247e)), closes [#486](#486) [#532](#532)
@markov00
Copy link
Member

🎉 This PR is included in version 18.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Apr 15, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gauge/Goal Chart type
3 participants