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: series identifications throughout library #419

Merged
merged 29 commits into from
Dec 2, 2019

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Oct 16, 2019

Summary

BREAKING CHANGE: GeometryId is now SeriesIdentifier (see below for type)

customSeriesColors prop on SeriesSpec which used to take a CustomSeriesColorsMap but now expects a CustomSeriesColors type

The CustomSeriesColors gives the user flexibility and full insight into what series is being assigned a given color.

export type SeriesColorsArray = string[]; // array of colors
export type SeriesColorAccessorFn = (seriesIdentifier: SeriesIdentifier) => string | null;
export type CustomSeriesColors = SeriesColorsArray | SeriesColorAccessorFn;

Returning null will use the default color value.

LegendItemListener now passes the SeriesIdentifier type as the first callback argument

- export type LegendItemListener = (dataSeriesIdentifiers: DataSeriesColorsValues | null) => void;
+ export type LegendItemListener = (series: SeriesIdentifier | null) => void;

Fixes: #325

If it involves visual changes include a screenshot or gif.

From now on we should use the more specific SeriesIdentifier type below in place of GeometryId to identify a particular series. This gives all the information to the consumer of the Chart to change anything for a given series (i.e. name, color, styles, hide/show, etc.)

export interface SeriesIdentifier {
  key: string;
  specId: SpecId;
  yAccessor: string | number;
  splitAccessors: Map<string | number, string | number>;
  seriesKeys: any[];
}

Before

const barCustomSeriesColors: CustomSeriesColorsMap = new Map();
const barDataSeriesColorValues: DataSeriesColorsValues = {
  colorValues: ['cloudflare.com', 'direct-cdn', 'y2'],
  specId: getSpecId('bars'),
};
barCustomSeriesColors.set(barDataSeriesColorValues, 'aquamarine');

const data = [
  { x: 0, g1: 'cdn.google.com', g2: 'direct-cdn', y1: 1, y2: 4 },
  // ...
]

return (
  <Chart>
    {/* ... */}
    <BarSeries
      id={getSpecId('bars')}
      xAccessor="x"
      yAccessors={['y1', 'y2']}
      splitSeriesAccessors={['g1', 'g2']}
      customSeriesColors={barCustomSeriesColors}
      data={data}
    />
  </Chart>
);

After

As function accessor

const barSeriesColorAccesor: CustomSeriesColors = ({ specId, yAccessor, splitAccessors }) => {
  if (
    specId === getSpecId('bars') &&
    yAccessor === 'y1' &&
    splitAccessors.get('g1') === 'cloudflare.com' &&
    splitAccessors.get('g2') === 'direct-cdn'
  ) {
    return 'aquamarine';
  }

  return null;
};

const data = [
  { x: 0, g1: 'cdn.google.com', g2: 'direct-cdn', y1: 1, y2: 4 },
  // ...
]

return (
  <Chart>
    {/* ... */}
    <BarSeries
      id={getSpecId('bars')}
      xAccessor="x"
      yAccessors={['y1', 'y2']}
      splitSeriesAccessors={['g1', 'g2']}
      customSeriesColors={barSeriesColorAccesor}
      data={data}
    />
  </Chart>
);

As array of colors

The array of colors will chose a color from the array based on the count index. In the example below the first series in the spec with be 'red', second 'white' and so on.

const data = [
  { x: 0, g1: 'cdn.google.com', g2: 'direct-cdn', y1: 1, y2: 4 },
  // ...
]

return (
  <Chart>
    {/* ... */}
    <BarSeries
      id={getSpecId('bars')}
      xAccessor="x"
      yAccessors={['y1', 'y2']}
      splitSeriesAccessors={['g1', 'g2']}
      customSeriesColors={['red', 'white', 'blue']}
      data={data}
    />
  </Chart>
);

Internally all series are now identified with the seriesKey generated from the getSeriesKey method.

Change naming for all series to use SeriesIdentifier and SeriesIdentifier.key

Closes #325, #179 and #245

Checklist

  • 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

start refactor of series Id and tooltip visibility
@nickofthyme nickofthyme self-assigned this Oct 16, 2019
@nickofthyme nickofthyme changed the title fix: hide series in tooltip (#415) refactor: series identifications throughout library Oct 16, 2019
@nickofthyme nickofthyme mentioned this pull request Oct 16, 2019
4 tasks
@nickofthyme nickofthyme added the wip work in progress label Oct 16, 2019
@nickofthyme nickofthyme requested a review from markov00 October 17, 2019 22:02
@codecov-io
Copy link

codecov-io commented Oct 29, 2019

Codecov Report

Merging #419 into master will decrease coverage by 0.38%.
The diff coverage is 80.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
- Coverage   84.11%   83.72%   -0.39%     
==========================================
  Files         168      169       +1     
  Lines        4784     4849      +65     
  Branches      917      970      +53     
==========================================
+ Hits         4024     4060      +36     
- Misses        744      774      +30     
+ Partials       16       15       -1
Impacted Files Coverage Δ
src/specs/settings.tsx 100% <ø> (ø) ⬆️
..._types/xy_chart/renderer/canvas/bar_geometries.tsx 25.71% <ø> (ø) ⬆️
src/index.ts 100% <ø> (ø) ⬆️
...chart_types/xy_chart/utils/stacked_series_utils.ts 97.53% <ø> (ø) ⬆️
src/state/chart_state.ts 79.59% <ø> (ø) ⬆️
src/mocks/series/series.ts 81.96% <ø> (ø) ⬆️
src/utils/themes/theme.ts 100% <ø> (ø) ⬆️
...rt_types/xy_chart/utils/nonstacked_series_utils.ts 100% <ø> (ø) ⬆️
src/chart_types/xy_chart/utils/specs.ts 100% <ø> (ø) ⬆️
src/chart_types/xy_chart/renderer/dom/tooltips.tsx 69.69% <0%> (-2.18%) ⬇️
... and 26 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 3013c41...37e71e0. Read the comment docs.

@nickofthyme nickofthyme added :specs Chart specifications related issue enhancement New feature or request labels Oct 30, 2019
@rshen91 rshen91 marked this pull request as ready for review October 31, 2019 20:35
@nickofthyme nickofthyme removed the wip work in progress label Oct 31, 2019
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.

Blocking this until we fix the fact that different SeriesIdentifiers can possibly have the same Key

src/chart_types/xy_chart/utils/series.ts Show resolved Hide resolved
@markov00 markov00 self-requested a review November 28, 2019 16:33
.storybook/utils.ts Show resolved Hide resolved
@@ -18,7 +19,7 @@ export class DataGenerator {
return dataPoints;
}
generateGroupedSeries(totalPoints = 50, totalGroups = 2, groupPrefix = '') {
const groups = new Array(totalGroups).fill(0).map((group, i) => {
const groups = new Array(totalGroups).fill(0).map((__webpack_hash__, i) => { // eslint-disable-line
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is __webpack_hash__ ? An autogenerated hash?

Copy link
Member

Choose a reason for hiding this comment

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

???????? never so that before

Copy link
Member

Choose a reason for hiding this comment

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

I think you did it :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Really???? Idk @rshen91 do you know what this is ☝️?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what it is but I'm sure I accidentally imported it or something 😅.

Copy link
Contributor

Choose a reason for hiding this comment

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

Commit 74aa3f5 to remove!

stories/area_chart.tsx Show resolved Hide resolved
@markov00 markov00 merged commit 66a48ff into master Dec 2, 2019
@markov00 markov00 deleted the feat/series-identifiers branch December 2, 2019 15:59
@nickofthyme
Copy link
Collaborator Author

Yaaay!!! 🎉

markov00 pushed a commit that referenced this pull request Dec 2, 2019
# [15.0.0](v14.2.0...v15.0.0) (2019-12-02)

### Code Refactoring

* series identifications throughout library ([#419](#419)) ([66a48ff](66a48ff))
* use redux in favour of mobx ([#281](#281)) ([cd34716](cd34716))

### BREAKING CHANGES

* `GeometryId` is now `SeriesIdentifier`. `customSeriesColors` prop on `SeriesSpec` which used to take a `CustomSeriesColorsMap`, now expects a `CustomSeriesColors` type. `LegendItemListener` now passes the `SeriesIdentifier` type as the first callback argument.
* `SpecId`,`AxisId`, `AnnotationId` types are down-casted to a `string` type. The `getSpecId`, `getAxisId` and `getAnnotationId` methods still exist and but return just the same passed string until deprecated in a future version. The spec ids, previously `id`, `axisId`,`annotationId` etc are now aligned to use the same prop name: `id`. The chart rendering status `data-ech-render-complete` and `data-ech-render-count` is no more at the root level of the `echChart` div, but on its child element: `echChartStatus`. The `Spec` has two new private properties called `chartType` and `specType`.
@markov00
Copy link
Member

markov00 commented Dec 2, 2019

🎉 This PR is included in version 15.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Dec 2, 2019
markov00 added a commit that referenced this pull request Jan 29, 2020
This commit changes the object passed to the event listeners providing either the value of the
clicked/hovered element and the SeriesIdentifier object that can uniquely identify a series in elastic-charts. (see #419 for a description on SeriesIdentifiers)

BREAKING CHANGE: the `onElementOver` and the `onElementClick` are now called with
`Array<[GeometryValue, SeriesIdentifier]>` instead of `Array<GeometryValue>`

fix #505
markov00 pushed a commit that referenced this pull request Jan 30, 2020
# [17.0.0](v16.2.1...v17.0.0) (2020-01-30)

### Bug Fixes

* **brush:** rotate brush on rotated charts ([#528](#528)) ([985ac21](985ac21)), closes [#527](#527)

### Features

* text improvements ([#524](#524)) ([6e61700](6e61700))
* **listeners:** add seriesIdentifiers to element listeners ([#525](#525)) ([027d008](027d008)), closes [#419](#419) [#505](#505)

### BREAKING CHANGES

* **listeners:** the `onElementOver` and the `onElementClick` are now called with
`Array<[GeometryValue, SeriesIdentifier]>` instead of `Array<GeometryValue>`
* renames in `Partition` charts— `Layers`: `fillLabel.formatter`->`fillLabel.valueFormatter`; type `FillLabel`-> `FillLabelConfig`

Non-breaking changes:

* feat: the values in linked labels are rendered, just like they have been in the sectors (formerly, the value could optionally be put in the link label accessor itself)

* feat: font styling is possible separately for values: `valueFormatter` configs

* test: opacity decrease example; coloring examples

* feat: hierarchical data (`parent`, `sortIndex`) is made available to accessors (see stories, helpful with eg. coloring)

* refactor: tighter types; other code improvements
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [15.0.0](elastic/elastic-charts@v14.2.0...v15.0.0) (2019-12-02)

### Code Refactoring

* series identifications throughout library ([opensearch-project#419](elastic/elastic-charts#419)) ([fc37dd6](elastic/elastic-charts@fc37dd6))
* use redux in favour of mobx ([opensearch-project#281](elastic/elastic-charts#281)) ([50ba58c](elastic/elastic-charts@50ba58c))

### BREAKING CHANGES

* `GeometryId` is now `SeriesIdentifier`. `customSeriesColors` prop on `SeriesSpec` which used to take a `CustomSeriesColorsMap`, now expects a `CustomSeriesColors` type. `LegendItemListener` now passes the `SeriesIdentifier` type as the first callback argument.
* `SpecId`,`AxisId`, `AnnotationId` types are down-casted to a `string` type. The `getSpecId`, `getAxisId` and `getAnnotationId` methods still exist and but return just the same passed string until deprecated in a future version. The spec ids, previously `id`, `axisId`,`annotationId` etc are now aligned to use the same prop name: `id`. The chart rendering status `data-ech-render-complete` and `data-ech-render-count` is no more at the root level of the `echChart` div, but on its child element: `echChartStatus`. The `Spec` has two new private properties called `chartType` and `specType`.
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [17.0.0](elastic/elastic-charts@v16.2.1...v17.0.0) (2020-01-30)

### Bug Fixes

* **brush:** rotate brush on rotated charts ([opensearch-project#528](elastic/elastic-charts#528)) ([b6c3302](elastic/elastic-charts@b6c3302)), closes [opensearch-project#527](elastic/elastic-charts#527)

### Features

* text improvements ([opensearch-project#524](elastic/elastic-charts#524)) ([f7b53c8](elastic/elastic-charts@f7b53c8))
* **listeners:** add seriesIdentifiers to element listeners ([opensearch-project#525](elastic/elastic-charts#525)) ([643ef1b](elastic/elastic-charts@643ef1b)), closes [opensearch-project#419](elastic/elastic-charts#419) [opensearch-project#505](elastic/elastic-charts#505)

### BREAKING CHANGES

* **listeners:** the `onElementOver` and the `onElementClick` are now called with
`Array<[GeometryValue, SeriesIdentifier]>` instead of `Array<GeometryValue>`
* renames in `Partition` charts— `Layers`: `fillLabel.formatter`->`fillLabel.valueFormatter`; type `FillLabel`-> `FillLabelConfig`

Non-breaking changes:

* feat: the values in linked labels are rendered, just like they have been in the sectors (formerly, the value could optionally be put in the link label accessor itself)

* feat: font styling is possible separately for values: `valueFormatter` configs

* test: opacity decrease example; coloring examples

* feat: hierarchical data (`parent`, `sortIndex`) is made available to accessors (see stories, helpful with eg. coloring)

* refactor: tighter types; other code improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released Issue released publicly :specs Chart specifications related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide series on tooltip
4 participants