-
Notifications
You must be signed in to change notification settings - Fork 120
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
src/chart_types/goal_chart/state/selectors/is_tooltip_visible.ts
Outdated
Show resolved
Hide resolved
src/chart_types/goal_chart/state/selectors/on_element_click_caller.ts
Outdated
Show resolved
Hide resolved
src/chart_types/goal_chart/state/selectors/on_element_out_caller.ts
Outdated
Show resolved
Hide resolved
src/chart_types/goal_chart/state/selectors/on_element_over_caller.ts
Outdated
Show resolved
Hide resolved
stories/goal/goal.stories.tsx
Outdated
export { example as spartanGoal } from './5_spartan'; | ||
export { example as spartanHorizontal } from './6_spartan_horizontal'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const circular = subtype === 'goal'; | ||
const vertical = subtype === 'verticalBullet'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Deferred feedback items so far: |
There was a problem hiding this 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 = { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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} |
There was a problem hiding this comment.
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.
# [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)
🎉 This PR is included in version 18.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [18.3.0](elastic/elastic-charts@v18.2.2...v18.3.0) (2020-04-15) ### Bug Fixes * remove series with undefined splitSeriesAccessor values ([opensearch-project#627](elastic/elastic-charts#627)) ([e8bd5ea](elastic/elastic-charts@e8bd5ea)) ### Features * gauge, goal and bullet graph (alpha) ([opensearch-project#614](elastic/elastic-charts#614)) ([4191664](elastic/elastic-charts@4191664)) * **partition:** add legend and highlighters ([opensearch-project#616](elastic/elastic-charts#616)) ([b939596](elastic/elastic-charts@b939596)), closes [opensearch-project#486](elastic/elastic-charts#486) [opensearch-project#532](elastic/elastic-charts#532)
Summary
Alpha version for goal, gauge and bullet graph (original is designed by Stephen Few). It can already generate these:
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:
ticks
needs to be a list of numbersCloses #520
Checklist
Delete any items that are not applicable to this PR.
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[ ] Unit tests were updated or added to match the most common scenarios