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

refactor: add manual sorting for tooltip, legend and rendering #924

Merged
merged 20 commits into from
Jan 25, 2021

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Nov 30, 2020

Summary

fixes #303

Adds a way to sort tooltip, legend and rendered series via chart specification:

The API is currently hidden and not exposed to the final consumer to allow us cleaning this feature once we have found a better type for the SeriesIdentifier.
Although the API is hidden, you can ignore ts errors and using it at your own risk using the following hidden API and types:

export interface SettingsSpec extends Spec {
  /**
   * A compare function or an object of compare functions to sort
   * series in a different part of the chart like tooltip, legend and
   * the rendering order on the screen. To assign the same compare function.
   *  @defaultValue the series are sorted in order of appearance in the chart configuration
   */
  sortSeriesBy?: SeriesCompareFn | SortSeriesByConfig;
}


/**
 * An object of compare functions to sort
 * series in different part of the chart like tooltip, legend and rendering order.
 */
export interface SortSeriesByConfig {
  /**
   * A SeriesSortFn to sort the legend values (top-bottom)
   * It has precedence over the general one
   */
  legend?: SeriesCompareFn;
  /**
   * A SeriesSortFn to sort tooltip values (top-bottom)
   * It has precedence over the general one
   */
  tooltip?: SeriesCompareFn;
  /**
   * A SeriesSortFn to sort the rendering order of series.
   * Left/right for cluster, bottom-up for stacked.
   * It has precedence over the general one
   * Currently available only on XY charts
   */
  rendering?: SeriesCompareFn;
  /**
   * The default SeriesSortFn in case no other specific sorting fn are used.
   * The rendering sorting is applied only to XY charts at the moment
   */
  default?: SeriesCompareFn;
}

export type SeriesCompareFn = (siA: SeriesIdentifier, siB: SeriesIdentifier) => number;

An example of usage is:

...
// sort ascending by group and yAccessor (a,y1)(a,y2)(b,y1)(b,y2)
function sortSeriesFn(a: SeriesIdentifier, b: SeriesIdentifier) {
  const { splitAccessors: aSplit, yAccessor: aYAccessor, specId: specIdA } = a as XYChartSeriesIdentifier;
  const { splitAccessors: bSplit, yAccessor: bYAccessor, specId: specIdB } = b as XYChartSeriesIdentifier;
  const aGroup = `${aSplit.get('g') ?? ''}`;
  const bGroup = `${bSplit.get('g') ?? ''}`;

  // sort by group descending within stacked bars
  if (specIdA === specIdB && specIdA === 'stacked') {
    return bGroup.localeCompare(aGroup);
  }
  // sort by by Yaccessor with same group
  if (aGroup === bGroup) {
    return aYAccessor.localeCompare(bYAccessor);
  }
  return aGroup.localeCompare(bGroup);
}
...
  <Settings
      sortSeriesBy={sortSeriesFn}
    />
 

Notes

The VRTs changes are changes only to the orders of elements in the legend and tooltip

@codecov-io
Copy link

codecov-io commented Nov 30, 2020

Codecov Report

Merging #924 (2782581) into master (944ac6c) will increase coverage by 0.05%.
The diff coverage is 89.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #924      +/-   ##
==========================================
+ Coverage   70.85%   70.90%   +0.05%     
==========================================
  Files         346      350       +4     
  Lines       11008    11072      +64     
  Branches     2424     2437      +13     
==========================================
+ Hits         7800     7851      +51     
- Misses       3195     3208      +13     
  Partials       13       13              
Flag Coverage Δ
unittests 70.90% <89.04%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
...xy_chart/state/selectors/compute_series_domains.ts 100.00% <ø> (ø)
src/chart_types/xy_chart/utils/specs.ts 100.00% <ø> (ø)
src/specs/index.ts 100.00% <ø> (ø)
src/specs/settings.tsx 87.09% <ø> (ø)
.../selectors/get_tooltip_values_highlighted_geoms.ts 87.05% <55.55%> (-3.74%) ⬇️
...chart_types/xy_chart/utils/stacked_series_utils.ts 88.40% <66.66%> (-1.15%) ⬇️
...chart_types/xy_chart/state/utils/get_last_value.ts 75.00% <75.00%> (ø)
...art_types/xy_chart/utils/default_series_sort_fn.ts 83.33% <83.33%> (ø)
src/utils/series_sort.ts 86.66% <86.66%> (ø)
src/chart_types/xy_chart/domains/y_domain.ts 97.91% <100.00%> (+2.12%) ⬆️
... and 12 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 944ac6c...2782581. Read the comment docs.

@markov00 markov00 changed the title feat: add manual sorting for tooltip, legend and rendering refactor: add manual sorting for tooltip, legend and rendering Jan 20, 2021
@markov00 markov00 marked this pull request as ready for review January 20, 2021 17:48
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Reviewed all the code and have a few comments. Looking at the screenshots and storybook next.

api/charts.api.md Show resolved Hide resolved
src/chart_types/xy_chart/state/utils/utils.ts Outdated Show resolved Hide resolved
src/chart_types/xy_chart/utils/scales.test.ts Show resolved Hide resolved
src/chart_types/xy_chart/utils/series.ts Outdated Show resolved Hide resolved
src/specs/settings.tsx Outdated Show resolved Hide resolved
stories/bar/47_stacked_only_grouped.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

All screenshots and storybook stories look good 🎉

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

@markov00 markov00 merged commit fcc4994 into elastic:master Jan 25, 2021
@markov00 markov00 deleted the 2020_11_30-data_sorting branch January 25, 2021 18:25
@markov00
Copy link
Member Author

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jan 30, 2021
@nickofthyme nickofthyme mentioned this pull request Feb 4, 2021
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.

The order of the series in the tooltip does not match the legend
3 participants