Skip to content

Commit

Permalink
use display enum instead of inline boolean
Browse files Browse the repository at this point in the history
  • Loading branch information
thompsongl committed Sep 13, 2019
1 parent 5d6fe36 commit 3453210
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 7 deletions.
7 changes: 4 additions & 3 deletions src-docs/src/views/color_picker/color_picker_example.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ const inlineSnippet = `<EuiColorPicker
onChange={handleChange}
color={chosenColor}
isInvalid={hasErrors}
inline={true}
display="inline"
/>
`;

Expand Down Expand Up @@ -240,8 +240,9 @@ export const ColorPickerExample = {
],
text: (
<p>
Use the <EuiCode>inline</EuiCode> prop to display the color picker
without an input or popover.
Set the <EuiCode>display</EuiCode> prop to `inline` to display the
color picker without an input or popover. Note that the{' '}
<EuiCode>button</EuiCode> prop will be ignored in this case.
</p>
),
snippet: inlineSnippet,
Expand Down
2 changes: 1 addition & 1 deletion src-docs/src/views/color_picker/inline.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export class Inline extends Component {
onChange={this.handleChange}
color={this.state.color}
isInvalid={hasErrors}
inline={true}
display="inline"
/>
);
}
Expand Down
2 changes: 1 addition & 1 deletion src/components/color_picker/color_picker.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ test('renders inline EuiColorPicker', () => {
<EuiColorPicker
onChange={onChange}
color="#ffeedd"
inline={true}
display="inline"
{...requiredProps}
/>
);
Expand Down
6 changes: 4 additions & 2 deletions src/components/color_picker/color_picker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
import { EuiHue } from './hue';
import { EuiSaturation } from './saturation';

type EuiColorPickerDisplay = 'default' | 'inline';

This comment has been minimized.

Copy link
@cchaos

cchaos Sep 13, 2019

Contributor

I know we're mincing words here, but what if default changes? I'd be more explicit and say 'popover' | 'inline'

This comment has been minimized.

Copy link
@thompsongl

thompsongl Sep 13, 2019

Author Contributor

I considered this. popover seems too generic because in the upcoming 'square' version, the popover is still used. And input is misleading because consumers have the option to completely change the element.

Changing what the default display type is (regardless of what we name it) would be a breaking change and we'd have to update use cases anyway.

This comment has been minimized.

Copy link
@cchaos

cchaos Sep 13, 2019

Contributor

True, I didn't think about the square version.

type EuiColorPickerMode = 'default' | 'swatch' | 'picker';

interface HTMLDivElementOverrides {
Expand All @@ -56,6 +57,7 @@ export interface EuiColorPickerProps
* Use the compressed style for EuiFieldText
*/
compressed?: boolean;
display?: EuiColorPickerDisplay;
disabled?: boolean;
fullWidth?: boolean;
id?: string;
Expand Down Expand Up @@ -94,9 +96,9 @@ export const EuiColorPicker: FunctionComponent<EuiColorPickerProps> = ({
color,
compressed = false,
disabled,
display = 'default',
fullWidth = false,
id,
inline = false,
isInvalid,
mode = 'default',
onBlur,
Expand Down Expand Up @@ -354,7 +356,7 @@ export const EuiColorPicker: FunctionComponent<EuiColorPickerProps> = ({
);
}

return inline ? (
return display === 'inline' ? (
<div className={classes}>{composite}</div>
) : (
<EuiPopover
Expand Down

0 comments on commit 3453210

Please sign in to comment.