From 4a2905de97bc98d1bb9dfba6ec1030b272fe3aac Mon Sep 17 00:00:00 2001 From: Andrej Ocenas Date: Mon, 11 Mar 2019 23:22:56 +0100 Subject: [PATCH 1/4] Move ColorPicker trigger to separate component and cleanup css --- .../components/ColorPicker/ColorPicker.tsx | 16 ++---- .../ColorPicker/ColorPickerTrigger.tsx | 56 +++++++++++++++++++ .../components/ColorPicker/_ColorPicker.scss | 53 ------------------ .../ThresholdsEditor/_ThresholdsEditor.scss | 1 - 4 files changed, 60 insertions(+), 66 deletions(-) create mode 100644 packages/grafana-ui/src/components/ColorPicker/ColorPickerTrigger.tsx diff --git a/packages/grafana-ui/src/components/ColorPicker/ColorPicker.tsx b/packages/grafana-ui/src/components/ColorPicker/ColorPicker.tsx index 5a6ddcd01b9..4602da9afc3 100644 --- a/packages/grafana-ui/src/components/ColorPicker/ColorPicker.tsx +++ b/packages/grafana-ui/src/components/ColorPicker/ColorPicker.tsx @@ -6,6 +6,7 @@ import { getColorFromHexRgbOrName } from '../../utils/namedColorsPalette'; import { SeriesColorPickerPopover } from './SeriesColorPickerPopover'; import { withTheme } from '../../themes/ThemeContext'; +import { ColorPickerTrigger } from './ColorPickerTrigger'; export const colorPickerFactory = ( popover: React.ComponentType, @@ -51,21 +52,12 @@ export const colorPickerFactory = ( onMouseLeave: hidePopper, }) ) : ( -
-
-
-
-
+ color={getColorFromHexRgbOrName(this.props.color || '#000000', theme.type)} + /> )} ); diff --git a/packages/grafana-ui/src/components/ColorPicker/ColorPickerTrigger.tsx b/packages/grafana-ui/src/components/ColorPicker/ColorPickerTrigger.tsx new file mode 100644 index 00000000000..6d4775d1739 --- /dev/null +++ b/packages/grafana-ui/src/components/ColorPicker/ColorPickerTrigger.tsx @@ -0,0 +1,56 @@ +import React, { forwardRef, Ref } from 'react'; + +interface ColorPickerTriggerProps { + onClick: () => void; + onMouseLeave: () => void; + color: string; +} + +export const ColorPickerTrigger = forwardRef(function ColorPickerTrigger( + props: ColorPickerTriggerProps, + ref: Ref +) { + return ( +
+
+
+
+
+ ); +}); diff --git a/packages/grafana-ui/src/components/ColorPicker/_ColorPicker.scss b/packages/grafana-ui/src/components/ColorPicker/_ColorPicker.scss index b07fe2433c9..626a03538bb 100644 --- a/packages/grafana-ui/src/components/ColorPicker/_ColorPicker.scss +++ b/packages/grafana-ui/src/components/ColorPicker/_ColorPicker.scss @@ -161,59 +161,6 @@ $arrowSize: 15px; flex-grow: 1; } -.sp-replacer { - background: inherit; - border: none; - color: inherit; - padding: 0; - border-radius: 10px; - cursor: pointer; -} - -.sp-replacer:hover, -.sp-replacer.sp-active { - border-color: inherit; - color: inherit; -} - -.sp-container { - border-radius: 0; - background-color: $dropdownBackground; - border: none; - padding: 0; -} - -.sp-palette-container, -.sp-picker-container { - border: none; -} - -.sp-dd { - display: none; -} - -.sp-preview { - position: relative; - width: 15px; - height: 15px; - border: none; - margin: 0; - float: left; - z-index: 0; - background-image: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAwAAAAMCAIAAADZF8uwAAAAGUlEQVQYV2M4gwH+YwCGIasIUwhT25BVBADtzYNYrHvv4gAAAABJRU5ErkJggg==); -} - -.sp-preview-inner, -.sp-alpha-inner, -.sp-thumb-inner { - display: block; - position: absolute; - top: 0; - left: 0; - bottom: 0; - right: 0; -} - .gf-color-picker__body { padding-bottom: $arrowSize; padding-left: 6px; diff --git a/packages/grafana-ui/src/components/ThresholdsEditor/_ThresholdsEditor.scss b/packages/grafana-ui/src/components/ThresholdsEditor/_ThresholdsEditor.scss index 296b082dc84..80a937ae0bc 100644 --- a/packages/grafana-ui/src/components/ThresholdsEditor/_ThresholdsEditor.scss +++ b/packages/grafana-ui/src/components/ThresholdsEditor/_ThresholdsEditor.scss @@ -86,7 +86,6 @@ .thresholds-row-input-inner-color-colorpicker { border-radius: 10px; - overflow: hidden; display: flex; align-items: center; box-shadow: 0 1px 4px rgba(0, 0, 0, 0.25); From 96285c4f5695e6a4e70f95a9889d0632bfa2fb0e Mon Sep 17 00:00:00 2001 From: Andrej Ocenas Date: Tue, 12 Mar 2019 22:37:55 +0100 Subject: [PATCH 2/4] Use render props pattern in color picker --- .../ColorPicker/ColorPicker.story.tsx | 11 +++++++++- .../components/ColorPicker/ColorPicker.tsx | 21 ++++++++++++------- .../ColorPicker/ColorPickerPopover.tsx | 2 +- .../ColorPicker/ColorPickerTrigger.tsx | 4 ++-- .../panel/graph/Legend/LegendSeriesItem.tsx | 8 ++++--- 5 files changed, 32 insertions(+), 14 deletions(-) diff --git a/packages/grafana-ui/src/components/ColorPicker/ColorPicker.story.tsx b/packages/grafana-ui/src/components/ColorPicker/ColorPicker.story.tsx index f7fa71391ae..c4f2a848029 100644 --- a/packages/grafana-ui/src/components/ColorPicker/ColorPicker.story.tsx +++ b/packages/grafana-ui/src/components/ColorPicker/ColorPicker.story.tsx @@ -50,7 +50,16 @@ ColorPickerStories.add('Series color picker', () => { color={selectedColor} onChange={color => updateSelectedColor(color)} > -
Open color picker
+ {({ ref, showColorPicker, hideColorPicker }) => ( +
+ Open color picker +
+ )} ); }} diff --git a/packages/grafana-ui/src/components/ColorPicker/ColorPicker.tsx b/packages/grafana-ui/src/components/ColorPicker/ColorPicker.tsx index 4602da9afc3..4a46458a115 100644 --- a/packages/grafana-ui/src/components/ColorPicker/ColorPicker.tsx +++ b/packages/grafana-ui/src/components/ColorPicker/ColorPicker.tsx @@ -1,4 +1,5 @@ import React, { Component, createRef } from 'react'; +import { omit } from 'lodash'; import { PopperController } from '../Tooltip/PopperController'; import { Popper } from '../Tooltip/Popper'; import { ColorPickerPopover, ColorPickerProps, ColorPickerChangeHandler } from './ColorPickerPopover'; @@ -8,13 +9,19 @@ import { SeriesColorPickerPopover } from './SeriesColorPickerPopover'; import { withTheme } from '../../themes/ThemeContext'; import { ColorPickerTrigger } from './ColorPickerTrigger'; +type ColorPickerTriggerRenderer = (props: { + ref: React.RefObject; + showColorPicker: () => void; + hideColorPicker: () => void; +}) => React.ReactNode; + export const colorPickerFactory = ( popover: React.ComponentType, displayName = 'ColorPicker' ) => { - return class ColorPicker extends Component { + return class ColorPicker extends Component { static displayName = displayName; - pickerTriggerRef = createRef(); + pickerTriggerRef = createRef(); onColorChange = (color: string) => { const { onColorChange, onChange } = this.props; @@ -24,11 +31,11 @@ export const colorPickerFactory = ( }; render() { + const { theme, children } = this.props; const popoverElement = React.createElement(popover, { - ...this.props, + ...omit(this.props, 'children'), onChange: this.onColorChange, }); - const { theme, children } = this.props; return ( @@ -46,10 +53,10 @@ export const colorPickerFactory = ( )} {children ? ( - React.cloneElement(children as JSX.Element, { + (children as ColorPickerTriggerRenderer)({ ref: this.pickerTriggerRef, - onClick: showPopper, - onMouseLeave: hidePopper, + showColorPicker: showPopper, + hideColorPicker: hidePopper, }) ) : ( extends ColorPickerProps, PopperContentProps { customPickers?: T; } diff --git a/packages/grafana-ui/src/components/ColorPicker/ColorPickerTrigger.tsx b/packages/grafana-ui/src/components/ColorPicker/ColorPickerTrigger.tsx index 6d4775d1739..0445fd465e3 100644 --- a/packages/grafana-ui/src/components/ColorPicker/ColorPickerTrigger.tsx +++ b/packages/grafana-ui/src/components/ColorPicker/ColorPickerTrigger.tsx @@ -1,4 +1,4 @@ -import React, { forwardRef, Ref } from 'react'; +import React, { forwardRef } from 'react'; interface ColorPickerTriggerProps { onClick: () => void; @@ -8,7 +8,7 @@ interface ColorPickerTriggerProps { export const ColorPickerTrigger = forwardRef(function ColorPickerTrigger( props: ColorPickerTriggerProps, - ref: Ref + ref: React.Ref ) { return (
- - - + {({ ref, showColorPicker, hideColorPicker }) => ( + + + + )} ); } From dfb2dd2500d8c4a3011f629f95fd11ac008039ea Mon Sep 17 00:00:00 2001 From: Andrej Ocenas Date: Tue, 12 Mar 2019 23:23:57 +0100 Subject: [PATCH 3/4] Add simple test for the ColorPicker --- .../ColorPicker/ColorPicker.test.tsx | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 packages/grafana-ui/src/components/ColorPicker/ColorPicker.test.tsx diff --git a/packages/grafana-ui/src/components/ColorPicker/ColorPicker.test.tsx b/packages/grafana-ui/src/components/ColorPicker/ColorPicker.test.tsx new file mode 100644 index 00000000000..8bf41f307f3 --- /dev/null +++ b/packages/grafana-ui/src/components/ColorPicker/ColorPicker.test.tsx @@ -0,0 +1,23 @@ +import React from 'react'; +import renderer from 'react-test-renderer'; +import { ColorPicker } from './ColorPicker'; +import { ColorPickerTrigger } from './ColorPickerTrigger'; + +describe('ColorPicker', () => { + it('renders ColorPickerTrigger component by default', () => { + expect( + renderer.create( {}} />).root.findByType(ColorPickerTrigger) + ).toBeTruthy(); + }); + + it('renders custom trigger when supplied', () => { + const div = renderer + .create( + {}}> + {() =>
Custom trigger
} +
+ ) + .root.findByType('div'); + expect(div.children[0]).toBe('Custom trigger'); + }); +}); From fc199a35cacf1eab84031b4e10ff56cfbbdd98e7 Mon Sep 17 00:00:00 2001 From: Andrej Ocenas Date: Tue, 12 Mar 2019 23:36:51 +0100 Subject: [PATCH 4/4] Add comments --- .../src/components/ColorPicker/ColorPicker.tsx | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/grafana-ui/src/components/ColorPicker/ColorPicker.tsx b/packages/grafana-ui/src/components/ColorPicker/ColorPicker.tsx index 4a46458a115..fd5f81c3f33 100644 --- a/packages/grafana-ui/src/components/ColorPicker/ColorPicker.tsx +++ b/packages/grafana-ui/src/components/ColorPicker/ColorPicker.tsx @@ -9,7 +9,15 @@ import { SeriesColorPickerPopover } from './SeriesColorPickerPopover'; import { withTheme } from '../../themes/ThemeContext'; import { ColorPickerTrigger } from './ColorPickerTrigger'; +/** + * If you need custom trigger for the color picker you can do that with a render prop pattern and supply a function + * as a child. You will get show/hide function which you can map to desired interaction (like onClick or onMouseLeave) + * and a ref which needs to be passed to an HTMLElement for correct positioning. If you want to use class or functional + * component as a custom trigger you will need to forward the reference to first HTMLElement child. + */ type ColorPickerTriggerRenderer = (props: { + // This should be a React.RefObject but due to how object refs are defined you cannot downcast from that + // to a specific type like React.RefObject even though it would be fine in runtime. ref: React.RefObject; showColorPicker: () => void; hideColorPicker: () => void; @@ -53,6 +61,9 @@ export const colorPickerFactory = ( )} {children ? ( + // Children have a bit weird type due to intersection used in the definition so we need to cast here, + // but the definition is correct and should not allow to pass a children that does not conform to + // ColorPickerTriggerRenderer type. (children as ColorPickerTriggerRenderer)({ ref: this.pickerTriggerRef, showColorPicker: showPopper,