feat: allow use of functions for y, y0, split and stack accessors by nickofthyme · Pull Request #943 · elastic/elastic-charts

nickofthyme · 2020-12-08T21:17:23Z

Summary

closes #808, closes #838, related to #837

Allows for functional accessors (AccessorFn) on yAccessors, y0Accessors, splitSeriesAccessors and stackAccessors.

Note: The stackAccessors values are not yet used, currently treated as boolean value (i.e. length > 0)

<BarSeries
  id="bars1"
  xAccessor={(d) => d.x}
  yAccessors={[(d) => d.y]}
  splitSeriesAccessors={[(d) => d.g]}
  data={data}
/>

Complex example with `seriesName`

const yAccessorFn: AccessorFn = ({ range }) => `${range.to} - ${range.from}`;
yAccessorFn.seriesName = 'complex y value';

If no seriesName is provided, the default value will be set using the index (index:0).

Checklist

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
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

codecov-io · 2020-12-08T23:08:45Z

Codecov Report

Merging #943 (81b798f) into master (3c823fd) will increase coverage by 0.53%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master     #943      +/-   ##
==========================================
+ Coverage   70.13%   70.66%   +0.53%     
==========================================
  Files         342      359      +17     
  Lines       11026    11342     +316     
  Branches     2391     2416      +25     
==========================================
+ Hits         7733     8015     +282     
- Misses       3279     3307      +28     
- Partials       14       20       +6

Flag	Coverage Δ
unittests	`70.14% <77.77%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
src/chart_types/xy_chart/utils/specs.ts	`100.00% <ø> (ø)`
src/utils/accessor.ts	`81.81% <ø> (ø)`
src/chart_types/xy_chart/utils/series.ts	`83.39% <77.77%> (-0.25%)`	⬇️
src/mocks/scale/index.ts	`100.00% <0.00%> (ø)`
src/mocks/utils.ts	`100.00% <0.00%> (ø)`
src/mocks/index.ts	`100.00% <0.00%> (ø)`
src/mocks/store/store.ts	`92.59% <0.00%> (ø)`
src/mocks/annotations/annotations.ts	`73.33% <0.00%> (ø)`
src/mocks/series/utils.ts	`100.00% <0.00%> (ø)`
src/mocks/theme.ts	`100.00% <0.00%> (ø)`
... and 10 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 3c823fd...b75b8ca. Read the comment docs.

markov00

Thanks Nick for taking the ownership of this feature.
It looks great, I've written few comments I'd like to discuss before merging

markov00 · 2020-12-09T11:33:30Z

src/utils/accessor.ts

 */
-export type UnaryAccessorFn<Return = any> = (datum: Datum) => Return;
+export interface UnaryAccessorFn<Return = any> {
+  seriesName?: string;


seriesName doesn't sound related to the accessor function. A series describe one or multiple objects made up of one or multiple fields and values. Here instead you are requesting a name for an accessor.
We can also barely specify it as a name or fieldName

Yeah good point. I don't wanna use name because they could unknowingly set the name based on the variable name of the function itself. See 4b6876c

markov00 · 2020-12-09T11:34:14Z

src/chart_types/xy_chart/utils/series.ts

+ * @param index
+ * @internal
+ */
+export function getAccessorString(accessor: Accessor | AccessorFn, index: number) {


Similar to the comment relate to the accessor seriesName, this can be renamed to something like:
getAccessorName or getAccessorFieldName

markov00 · 2020-12-09T11:38:31Z

stories/bar/54_functional_accessors.tsx

+yAccessorFn.seriesName = 'simple y value';
+
+// complex example
+const yAccessorFn: AccessorFn = ({ range }) => \`\${range.to} - \${range.from}\`;
+yAccessorFn.seriesName = 'complex y value';
+\`\`\`
+
+If no \`seriesName\` is provided, the default value will be set using the index \`(index:0)\`.
+
+Try changing the \`seriesName\` for the y and split accessor functions in the storybook knobs.


In case you change the seriesName field, remember to change also this

markov00 · 2020-12-09T12:06:46Z

src/chart_types/xy_chart/utils/series.ts

+        const accessorStr = getAccessorString(accessor, index);
+        splitAccessors.set(accessorStr, value);


Here there can be an issue when the seriesName of an accessor function is the same as the key of another accessor.
On the provided story, if you use splitAccessorFn.seriesName = 'g1 the splitSeriesAccessors={['g1', splitAccessorFn]} will wrongly split the data.
I'm not sure if we need to prevent this or just discourage this practice.

Yeah, that's tough to avoid. I think it's the same with duplicate Accessor strings in that we cannot really avoid it. I added a note to the story but maybe we can figure out a better way in the future (81b798f)

# [24.4.0](v24.3.0...v24.4.0) (2020-12-09) ### Bug Fixes * empty labels on debug state ([#940](#940)) ([3c823fd](3c823fd)) ### Features * allow use of functions for y, y0, split and stack accessors ([#943](#943)) ([22425d3](22425d3))

markov00 · 2020-12-09T17:39:51Z

🎉 This PR is included in version 24.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [24.4.0](elastic/elastic-charts@v24.3.0...v24.4.0) (2020-12-09) ### Bug Fixes * empty labels on debug state ([opensearch-project#940](elastic/elastic-charts#940)) ([3d1281b](elastic/elastic-charts@3d1281b)) ### Features * allow use of functions for y, y0, split and stack accessors ([opensearch-project#943](elastic/elastic-charts#943)) ([e881723](elastic/elastic-charts@e881723))

feat: allow use of functions for y, y0, split and stack accessors

01570e1

nickofthyme added enhancement New feature or request :data Data/series/scales related issue :xy Bar/Line/Area chart related labels Dec 8, 2020

nickofthyme requested review from markov00, monfera and rshen91 December 8, 2020 21:17

nickofthyme added 4 commits December 8, 2020 15:17

Merge branch 'master' into feat/fn-accessors

57261f7

fix: api changes

08d53f8

test: add unit tests

dea4fb0

test: add visual regressions tests

079b377

markov00 reviewed Dec 9, 2020

View reviewed changes

nickofthyme added 3 commits December 9, 2020 09:08

chore: rename seriesName to feildName

4b6876c

chore: update story info

81b798f

fix: vrt screenshot file names

b75b8ca

nickofthyme merged commit 22425d3 into elastic:master Dec 9, 2020

nickofthyme deleted the feat/fn-accessors branch December 9, 2020 17:08

markov00 added the released Issue released publicly label Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow use of functions for y, y0, split and stack accessors#943

feat: allow use of functions for y, y0, split and stack accessors#943
nickofthyme merged 8 commits intoelastic:masterfrom
nickofthyme:feat/fn-accessors

nickofthyme commented Dec 8, 2020 •

edited

Loading

Uh oh!

codecov-io commented Dec 8, 2020 •

edited

Loading

Uh oh!

markov00 left a comment

Uh oh!

markov00 Dec 9, 2020

Uh oh!

nickofthyme Dec 9, 2020

Uh oh!

markov00 Dec 9, 2020

Uh oh!

nickofthyme Dec 9, 2020

Uh oh!

markov00 Dec 9, 2020

Uh oh!

nickofthyme Dec 9, 2020

Uh oh!

markov00 Dec 9, 2020

Uh oh!

nickofthyme Dec 9, 2020

Uh oh!

markov00 commented Dec 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		const accessorStr = getAccessorString(accessor, index);
		splitAccessors.set(accessorStr, value);

Conversation

nickofthyme commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Complex example with seriesName

Checklist

Uh oh!

codecov-io commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

markov00 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markov00 commented Dec 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nickofthyme commented Dec 8, 2020 •

edited

Loading

Complex example with `seriesName`

codecov-io commented Dec 8, 2020 •

edited

Loading