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(partition): order slices and sectors #1112

Merged
merged 10 commits into from
Apr 12, 2021

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Apr 9, 2021

Summary

Adds slice sorting for the use of pie charts and sunburst, which allows

  • placing a supplied "Other" slice to be placed at the end, even if, as in this example, the "Other" slice is larger than a regular slice
  • slice sorting eg. when the partitioning is along an ordinal variable
  • can be used for alphabetical sorting too, though it's almost always a practice to avoid

Precondition for mosaic plot #1113
Closes #1012
Contributes toward elastic/kibana#90179
Partially addresses #381

image

The newly introduced sortPredicate is for a sort predicate that compares order of any given two ArrayEntry objects. ArrayEntry has already been part of the public API, so we don't need to publish a new data type for this.

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
  • 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

@monfera monfera added the :partition Partition/PieChart/Donut/Sunburst/Treemap chart related label Apr 9, 2021
@monfera monfera requested review from markov00 and stratoula April 9, 2021 10:20
@monfera
Copy link
Contributor Author

monfera commented Apr 9, 2021

jenkins please test this

@codecov-io
Copy link

codecov-io commented Apr 9, 2021

Codecov Report

Merging #1112 (954dfb8) into master (ec17cb2) will increase coverage by 0.44%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1112      +/-   ##
==========================================
+ Coverage   72.03%   72.47%   +0.44%     
==========================================
  Files         381      397      +16     
  Lines       11919    12244     +325     
  Branches     2601     2651      +50     
==========================================
+ Hits         8586     8874     +288     
- Misses       3308     3337      +29     
- Partials       25       33       +8     
Flag Coverage Δ
unittests 72.05% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/utils/accessor.ts 81.81% <ø> (ø)
src/utils/common.ts 86.16% <ø> (ø)
...es/partition_chart/layout/utils/group_by_rollup.ts 88.00% <100.00%> (+0.12%) ⬆️
...hart_types/partition_chart/layout/utils/treemap.ts 98.57% <100.00%> (+0.06%) ⬆️
...tion_chart/layout/viewmodel/hierarchy_of_arrays.ts 100.00% <100.00%> (ø)
...ypes/partition_chart/layout/viewmodel/viewmodel.ts 85.08% <100.00%> (ø)
src/chart_types/partition_chart/specs/index.ts 53.84% <100.00%> (ø)
src/mocks/scale/index.ts 100.00% <0.00%> (ø)
src/mocks/scale/scale.ts 77.77% <0.00%> (ø)
src/mocks/series/utils.ts 100.00% <0.00%> (ø)
... and 13 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 ec17cb2...954dfb8. Read the comment docs.

@monfera monfera added the enhancement New feature or request label Apr 10, 2021
@stratoula
Copy link

@monfera it seems fine to me. What I want is:

  • sort by the metric (numeric). I think that I can do it by just changing the clockWiseoption.
  • sort alphabetically if the user requests it (will use the new sortPredicate for that)

@flash1293 I think it is fine for Lens too. wdyt?

@monfera
Copy link
Contributor Author

monfera commented Apr 12, 2021

@stratoula correct, and if you need special sorting, eg. you want to put the "Other" slice to the end, even if it's not the smallest slice, then you can rely on the sortPredicate too

@markov00 markov00 changed the title feat(partition): Ordered slices and sectors feat(partition): order slices and sectors Apr 12, 2021
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.

Tested locally with configurations of specialFirstInnermostSector , clockwiseSectors and legend. Works as expected

@monfera monfera merged commit 74df29b into elastic:master Apr 12, 2021
@flash1293
Copy link

Didn't test the code, but the API looks good to me - a function will give full flexibility which can be use to look up the row index the data table for "user defined" sorting

@monfera
Copy link
Contributor Author

monfera commented Apr 12, 2021

Thanks @markov00 for the review and for @stratoula and @flash1293 for looking at the API. I ended up merging relatively fast as this doesn't lock us in anyway, if there's post-merge or integration time feedback to act upon, and it clears the baseline for the upcoming partition related PRs (lower risk of merge conflict)

@monfera monfera mentioned this pull request Apr 12, 2021
3 tasks
nickofthyme pushed a commit that referenced this pull request Apr 13, 2021
# [28.1.0](v28.0.1...v28.1.0) (2021-04-13)

### Bug Fixes

* **legend:** sizing for short labels with scrollbar ([#1115](#1115)) ([6e1f223](6e1f223))
* **xy:** negative bar highlight and click ([#1109](#1109)) ([ec17cb2](ec17cb2)), closes [#1100](#1100)

### Features

* **a11y:** improve chart figure ([#1104](#1104)) ([815cf39](815cf39))
* **partition:** order slices and sectors ([#1112](#1112)) ([74df29b](74df29b))
* **partitions:** small multipies events pass on smAccessorValue ([#1106](#1106)) ([a3234fe](a3234fe))
* **xy:** optionally rounds the domain to nice values ([#1087](#1087)) ([f644cc4](f644cc4))
* **xy:** specify pixel and ratio width for bars ([#1114](#1114)) ([58de413](58de413))
* mosaic ([#1113](#1113)) ([64bdd88](64bdd88))
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 28.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Apr 13, 2021
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
enhancement New feature or request :partition Partition/PieChart/Donut/Sunburst/Treemap chart related released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Piechart slice order
6 participants