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: percentage display in partitioning charts #558

Merged
merged 19 commits into from
Mar 11, 2020

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Feb 19, 2020

Summary

Adds the option of using the ratio rather than the value in data that's the basis for aggregation.

The ratio is computed as the value of the sector or treemap square divided by the total sum of the value.

Client code may use the ratio to display the ratio, or a percentage, instead of the sector / rectangle value.

image
image

One item in #518

Checklist

Use strikethroughs to remove checklist items you don't feel are 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
  • Each commit follows the convention

@monfera monfera added the wip work in progress label Feb 19, 2020
@monfera monfera self-assigned this Feb 19, 2020
@codecov-io
Copy link

codecov-io commented Feb 19, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@103df02). Click here to learn what that means.
The diff coverage is 43.58%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #558   +/-   ##
=========================================
  Coverage          ?   70.23%           
=========================================
  Files             ?      208           
  Lines             ?     6238           
  Branches          ?     1208           
=========================================
  Hits              ?     4381           
  Misses            ?     1839           
  Partials          ?       18
Impacted Files Coverage Δ
...rtition_chart/layout/viewmodel/link_text_layout.ts 8.1% <ø> (ø)
...rtition_chart/layout/viewmodel/fill_text_layout.ts 7.69% <0%> (ø)
...es/partition_chart/layout/types/viewmodel_types.ts 80% <100%> (ø)
...t_types/partition_chart/state/selectors/tooltip.ts 31.81% <20%> (ø)
...es/partition_chart/layout/utils/group_by_rollup.ts 26.82% <40%> (ø)
...ypes/partition_chart/layout/viewmodel/viewmodel.ts 14.04% <50%> (ø)
...hart_types/partition_chart/layout/config/config.ts 66.66% <50%> (ø)
...ypes/partition_chart/state/selectors/scenegraph.ts 34.78% <60%> (ø)
src/chart_types/partition_chart/specs/index.ts 58.33% <66.66%> (ø)

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 103df02...d2e1b57. Read the comment docs.

@markov00
Copy link
Member

I've scanned the code and LGTM. I just want to make a small comment on the API:
your implementation impose to import a function, the percentageValueGetter and add them to the valueGetter props of the <Partition/> component. I think it's fine, but what if we provide a simpler version also? like allow the user to specify a string/constant or a function like

type ValueGetter = 'value' | 'percentage' | (node: ShapeTreeNode) => number;

This will simplify the API a bit and will make it a bit more readable in few cases

@monfera monfera removed the wip work in progress label Mar 5, 2020
@monfera monfera requested a review from markov00 March 5, 2020 17:32
@monfera
Copy link
Contributor Author

monfera commented Mar 5, 2020

image

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.

Everything looks good. Tested locally.
This actually reproduce the same behaviour in Kibana:

  • you can display percentage only or the number only on the slice label
  • you will see the formatted value plus the percentage in the tooltip

As a next step, we should make it configurable also on the slice

src/chart_types/partition_chart/state/selectors/tooltip.ts Outdated Show resolved Hide resolved
src/chart_types/partition_chart/layout/config/config.ts Outdated Show resolved Hide resolved
@markov00
Copy link
Member

Hey @monfera I've pushed a change to add the percentFormatter as part of the spec. This because Kibana has the freedom to change the percentage format depending on the Advanced settings. you can see the changes here: ae26259

I've also removed a small portion of code (the one related to the indices) on the tooltip selector, not sure what was the original requirements for these indices values, see: 1843a16

@monfera
Copy link
Contributor Author

monfera commented Mar 11, 2020

Thanks @marco the new commits look great. We can reconsider the datumIndices whenever the need comes back

@markov00
Copy link
Member

@monfera sure, let me copy the code here also:

    const datumIndices = new Set();
    pickedShapes.forEach((shape) => {
      const node = shape.parent;
      const shapeNode = node.children.find(([key]) => key === shape.dataName);
      if (shapeNode) {
        const indices = shapeNode[1][INPUT_KEY] || [];
        indices.forEach((i) => datumIndices.add(i));
      }
    });

@markov00 markov00 merged commit d6aa8d7 into elastic:master Mar 11, 2020
@markov00 markov00 mentioned this pull request Mar 11, 2020
93 tasks
markov00 pushed a commit that referenced this pull request Mar 17, 2020
# [18.0.0](v17.1.1...v18.0.0) (2020-03-17)

### Code Refactoring

* clean up TS types ([#554](#554)) ([22f7635](22f7635)), closes [#547](#547) [#547](#547)
* decouple tooltip from XY chart ([#553](#553)) ([e70792e](e70792e)), closes [#246](#246)

### Features

* cleaner color API on SeriesSpec ([#571](#571)) ([f769f7c](f769f7c))
* **legend:** allow color picker component render prop ([#545](#545)) ([90f4b95](90f4b95))
* **partition:** add element click, over and out events ([#578](#578)) ([103df02](103df02))
* **partition:** add tooltip ([#544](#544)) ([6bf9a69](6bf9a69)), closes [#246](#246)
* percentage display in partitioning charts ([#558](#558)) ([d6aa8d7](d6aa8d7))
* specify series name with a function on SeriesSpec ([#539](#539)) ([358455a](358455a))
* xAccessor can be a function accessor ([#574](#574)) ([bcc3d63](bcc3d63))

### BREAKING CHANGES

* The `getSpecId`, `getGroupId`, `getAxisId` and `getAnnotationId` are no longer available. Use a simple `string` instead.
* `customSeriesColors` prop on `SeriesSpec` is now `color`. The `CustomSeriesColors` type is  replaced with `SeriesColorAccessor`.
* Remove `customSubSeriesName` prop on series specs in favor of cleaner api using just the `name` prop on `SeriesSpec`. The types `SeriesStringPredicate`, `SubSeriesStringPredicate` have been removed.
* the `SeriesIdentifier` type is generalized into a simplified object with two values in common: `specId` and `key`. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`. The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra` and its default value is now `false` hiding the current/last value on the legend by default.
@markov00
Copy link
Member

🎉 This PR is included in version 18.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Mar 17, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [18.0.0](elastic/elastic-charts@v17.1.1...v18.0.0) (2020-03-17)

### Code Refactoring

* clean up TS types ([opensearch-project#554](elastic/elastic-charts#554)) ([6ce56fa](elastic/elastic-charts@6ce56fa)), closes [opensearch-project#547](elastic/elastic-charts#547) [opensearch-project#547](elastic/elastic-charts#547)
* decouple tooltip from XY chart ([opensearch-project#553](elastic/elastic-charts#553)) ([cb02cd0](elastic/elastic-charts@cb02cd0)), closes [opensearch-project#246](elastic/elastic-charts#246)

### Features

* cleaner color API on SeriesSpec ([opensearch-project#571](elastic/elastic-charts#571)) ([6a78c4e](elastic/elastic-charts@6a78c4e))
* **legend:** allow color picker component render prop ([opensearch-project#545](elastic/elastic-charts#545)) ([22ef1e6](elastic/elastic-charts@22ef1e6))
* **partition:** add element click, over and out events ([opensearch-project#578](elastic/elastic-charts#578)) ([4189573](elastic/elastic-charts@4189573))
* **partition:** add tooltip ([opensearch-project#544](elastic/elastic-charts#544)) ([0cffed4](elastic/elastic-charts@0cffed4)), closes [opensearch-project#246](elastic/elastic-charts#246)
* percentage display in partitioning charts ([opensearch-project#558](elastic/elastic-charts#558)) ([993a448](elastic/elastic-charts@993a448))
* specify series name with a function on SeriesSpec ([opensearch-project#539](elastic/elastic-charts#539)) ([fc6430b](elastic/elastic-charts@fc6430b))
* xAccessor can be a function accessor ([opensearch-project#574](elastic/elastic-charts#574)) ([f702e2c](elastic/elastic-charts@f702e2c))

### BREAKING CHANGES

* The `getSpecId`, `getGroupId`, `getAxisId` and `getAnnotationId` are no longer available. Use a simple `string` instead.
* `customSeriesColors` prop on `SeriesSpec` is now `color`. The `CustomSeriesColors` type is  replaced with `SeriesColorAccessor`.
* Remove `customSubSeriesName` prop on series specs in favor of cleaner api using just the `name` prop on `SeriesSpec`. The types `SeriesStringPredicate`, `SubSeriesStringPredicate` have been removed.
* the `SeriesIdentifier` type is generalized into a simplified object with two values in common: `specId` and `key`. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`. The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra` and its default value is now `false` hiding the current/last value on the legend by default.
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.

3 participants