-
Notifications
You must be signed in to change notification settings - Fork 120
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(legend): allow color picker component #545
feat(legend): allow color picker component #545
Conversation
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.
I've added few comments that needs to be addressed before going on with this feature
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.
LGTM, I've added few minor comments.
I think it will be great if you can also add a small example of the picker in storybook (maybe, you can just wait for #557 to be merged before adding it)
src/chart_types/xy_chart/state/selectors/get_tooltip_values_highlighted_geoms.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #545 +/- ##
=========================================
Coverage ? 72.09%
=========================================
Files ? 213
Lines ? 6197
Branches ? 1185
=========================================
Hits ? 4468
Misses ? 1710
Partials ? 19
Continue to review full report at Codecov.
|
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.
Changes LGTM, now I'm going through the manual test
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.
I think we should rethink the implementation, see my comments below
jenkins retest this |
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 great! Thanks for the cleanup on the docs also.
I've left just some very minor details comments
/** | ||
* Helper function to get highest override color. | ||
* | ||
* from highest to lowest: `temporary`, `customSeriesColors` then `persisted` |
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.
customSeriesColors
=> color
or customColors
clearTemporaryColors: () => void; | ||
setTemporaryColor: (key: SeriesKey, color: string) => void; | ||
setPersistedColor: (key: SeriesKey, color: string) => void; |
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.
clearTemporaryColors: () => void; | |
setTemporaryColor: (key: SeriesKey, color: string) => void; | |
setPersistedColor: (key: SeriesKey, color: string) => void; | |
clearTemporaryColors: typeof clearTemporaryColors; | |
setTemporaryColor: typeof setTemporaryColor; | |
setPersistedColor: typeof setPersistedColor; |
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.
I tried this but for some reason there was a type error last time, idk but it's fixed 👍
src/specs/settings.tsx
Outdated
anchor: HTMLDivElement; | ||
/** | ||
* Current color of the given series | ||
*/ | ||
color: string; |
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.
color: string; | |
color: Color; |
src/specs/settings.tsx
Outdated
/** | ||
* Callback to update temporary color state | ||
*/ | ||
onChange: (color: string) => void; |
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.
onChange: (color: string) => void; | |
onChange: (color: Color) => void; |
data={data} | ||
customSeriesColors={({ key }) => colors[key] ?? null} | ||
data={BARCHART_1Y1G} | ||
color={({ key }) => colors[key] ?? null} | ||
/> | ||
</Chart> | ||
); |
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.
Please add a description of the mechanism here:
example.story = {
parameters: {
info: {
text: 'Elastic charts will maintain the color selection in memory beyond chart updates. However, to persist colors beyond browser refresh the consumer would need to manage the color state and use the color prop on the` SeriesSpec` to assign a color via the `SeriesColorAccessor`.',
},
},
};
stories/legend/9_color_picker.tsx
Outdated
onChangeAction(color); | ||
onChange(color); |
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.
nit: just to keep consistency on the code between the handleChange and the handleClose
onChangeAction(color); | |
onChange(color); | |
onChange(color); | |
onChangeAction(color); |
- Add option to pass color picker component to chart - Allow picker prop to be react component or function component - Configure elastic charts to maintain color override state in memory - Generalize vrt common page object and add method for clicking on element
# [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
Add option to pass color picker component to chart.
Closes #538
with
FunctionComponent<LegendColorPickerProps>
or with
Component<LegendColorPickerProps>
where
The button argument is the legend item that should be used as the
button
prop forEuiPopover
as the positioning reference.Remove
seriesColorOverrides
logic which was deprecated, now unused, back when the color picker was removed from elastic charts.Demo usage with
EuiColorPicker
andEuiWrappingPopover
http://localhost:9001/?path=/story/legend--color-picker
Code
elastic-charts/stories/legend/9_color_picker.tsx
Lines 13 to 58 in 400a70b
Checklist
src/index.ts
(and stories only import from../src
except for test data & storybook)