-
Notifications
You must be signed in to change notification settings - Fork 122
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: add series name settings #539
feat: add series name settings #539
Conversation
Codecov Report
@@ Coverage Diff @@
## master #539 +/- ##
=======================================
Coverage 71.83% 71.83%
=======================================
Files 212 212
Lines 6142 6142
Branches 1181 1181
=======================================
Hits 4412 4412
Misses 1711 1711
Partials 19 19
Continue to review full report at Codecov.
|
Hi @nickofthyme I've few doubts on this PR because the same effect can be achieved with the available props like the following: customSeriesLabel={(args) => {
return args.seriesKeys.join(' ~~ ');
}} or customSeriesLabel={(args) => {
return args.seriesKeys.slice(0, args.seriesKeys.length -1).join(' ~~ ');
}} Users are always a bit reluctant to use Maps object (I remember most of them commenting on a simplified version) that could be in fact achieved with a simple object One other way we could think about this feature: what if we provide a set of utility function for this cases? function fullSeries(delimiter: string = ' - ') {
return (args) => {
return args.seriesKeys.join(delimiter);
}
}
function simpleSeries(delimiter: string = ' - ') {
return (args) => {
return args.seriesKeys.slice(0, args.seriesKeys.length -1).join(delimiter);
}
} this will allow much more configurability and doesn't pollute the, already polluted, set of props we have in the chart components. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Nick for that, seems everything fine except few small typos.
I've added few nit commits that can be ignored but i think it's useful if the discuss them
Move `Datum`, `Rotation`, `Position` and `Color` to `utils/commons`. Decouple legend from axis position method and move the `scales` to `utils/scales`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good, I've left few minor comments and one major related to the accessor type that should be resolved
This commit will decouple the tooltip component from the XY chart to allow Partition and other chart type an ease use. BREAKING CHANGE: 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. close elastic#246
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything LGTM, I've left few small comments
jenkins test this |
Refactor name prop on SeriesSpec to take custom naming props to name series. BREAKING CHANGE: 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.
# [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.
# [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.
Summary
Related to #537 (interim fix)
BREAKING CHANGE:
Remove
customSubSeriesName
prop on series specs in favor of cleaner api using just thename
prop onSeriesSpec
. The typesSeriesStringPredicate
,SubSeriesStringPredicate
have been removed.The
name
prop is defined below...This original function type definition is the same
However the
SeriesStringPredicate
is now a union type betweenSeriesNameFn
and the newgetSeriesNameFromOptions
type defined below.Mappings Examples
Replace yAccessors
ferrari - speed
ferrari - m/s
ferrari - acceleration
ferrari - m/s/s
porsche - speed
porsche - m/s
porsche - acceleration
porsche - m/s/s
Replace splitAccessors and reorder with
delimiter
ferrari - speed
speed : Ferrari
ferrari - acceleration
acceleration : Ferrari
porsche - speed
speed : Porsche
porsche - acceleration
acceleration : Porsche
Single series
ferrari
Ferrari
porsche
Porsche
See test cases for more examples
Checklist
src/index.ts
(and stories only import from../src
except for test data & storybook)