-
Notifications
You must be signed in to change notification settings - Fork 121
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); |
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)