Skip to content
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

[ColorPicker] Add hex/alpha text fields #2186

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions UNRELEASED.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@
### Enhancements

- Truncated long sort options in `ResourceList` ([#2957](https://github.com/Shopify/polaris-react/pull/2957)
- Added `hideTags` prop to `Filters` ([#2573](https://github.com/Shopify/polaris-react/pull/2573))
- Updated the type of the `title` prop in `ChoiceList` from `string` to `ReactNode` ([#2355](https://github.com/Shopify/polaris-react/pull/2355))
- Add text editor to the `ColorPicker` ([#2186](https://github.com/Shopify/polaris-react/pull/2186))

### Bug fixes

### Bug fixes

Expand Down
6 changes: 6 additions & 0 deletions locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@
"spinnerAccessibilityLabel": "Loading",
"connectedDisclosureAccessibilityLabel": "Related actions"
},

"ColorPicker": {
"textPickerAccessibilityLabel": "HEX color",
"alphaFieldAccessibilityLabel": "Alpha"
Comment on lines +31 to +32
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dpersing Any suggestions on what a11y labels should we use here?

The first is used for the hex/colour name/rgb input field and the second is used for the alpha percentage field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added these as placeholders for now.

},

"Common": {
"checkbox": "checkbox",
"undo": "Undo",
Expand Down
64 changes: 57 additions & 7 deletions src/components/ColorPicker/ColorPicker.scss
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
@import '../../styles/common';

$picker-size: rem(160px);
$picker-width: rem(160px);
$picker-with-alpha-width: rem(194px);
$picker-height: $picker-width;
$dragger-size: rem(18px);
$text-picker-width: rem(192px);
$text-picker-with-alpha-width: $picker-width;
$alpha-size: rem(90px);
$inner-shadow: inset 0 0 2px 0 rgba(0, 0, 0, 0.5);

$stacking-order: (
Expand All @@ -10,6 +15,10 @@ $stacking-order: (
dragger: 30,
);

$swatch-size: rem(20px);
$swatch-shadow: inset rgba(0, 0, 0, 0.07) 0 0 0 1px,
inset rgba(0, 0, 0, 0.15) 0 1px 3px 0;

@mixin checkers {
background-image: linear-gradient(
45deg,
Expand All @@ -36,8 +45,12 @@ $stacking-order: (
@include checkers;
position: relative;
overflow: hidden;
height: $picker-size;
width: $picker-size;
height: $picker-height;
width: $picker-width;

&.AlphaAllowed {
width: $picker-with-alpha-width;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we keep the same width for the main color, and reduce the width of the TextPicker?
Screen Shot 2020-03-19 at 2 50 07 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I just saw the earlier discussions about that too... I'm not sure increasing the width is really the best approach tho.
I understand why we would want the field to be large enough to fit a full rgb color, but it's main purpose is to render an hex value, so if we don't render the full rgb when the user writes it, I guess it's fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mirualves What do you think about this approach?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as the component width adapts to accept RGB colors comfortably, I don't see a problem.

}

// Need an extra pixel to avoid a small color bleed from happening
border-radius: var(--p-border-radius-base, border-radius() + 1px);
Expand Down Expand Up @@ -98,17 +111,17 @@ $stacking-order: (
$green: rgb(0, 255, 0);
$purple: rgb(255, 0, 255);
$huepicker-padding: $dragger-size;
$huepicker-bottom-padding-start: $picker-size - $dragger-size;
$huepicker-bottom-padding-start: $picker-width - $dragger-size;

.HuePicker,
.AlphaPicker {
position: relative;
overflow: hidden;
height: $picker-size;
height: $picker-height;
width: rem(24px);
margin-left: spacing(tight);
border-width: var(--p-border-radius-base, border-radius());
border-radius: $picker-size * 0.5;
border-radius: $picker-height * 0.5;
}

.HuePicker {
Expand All @@ -129,7 +142,7 @@ $huepicker-bottom-padding-start: $picker-size - $dragger-size;
@include checkers;

.ColorLayer {
border-radius: var(--p-override-none, $picker-size * 0.5);
border-radius: var(--p-override-none, $picker-height * 0.5);
}
}

Expand All @@ -149,3 +162,40 @@ $huepicker-bottom-padding-start: $picker-size - $dragger-size;
width: 100%;
cursor: pointer;
}

.TextFields {
display: flex;
}

.TextPicker {
width: $text-picker-width;
margin-top: spacing(tight);

&.AlphaAllowed {
width: $text-picker-with-alpha-width;
}
}

.TextFieldSwatch {
width: $swatch-size;
height: $swatch-size;
margin-left: -(spacing(extra-tight));
border-radius: 50%;
box-shadow: $swatch-shadow;

&.AlphaAllowed {
@include checkers;
}
}

.SwatchBackground {
width: 100%;
height: 100%;
border-radius: inherit;
box-shadow: inherit;
}

.AlphaField {
width: $alpha-size;
margin: spacing(tight) 0 0 spacing(tight);
}
133 changes: 102 additions & 31 deletions src/components/ColorPicker/ColorPicker.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,29 @@
import React from 'react';
import debounce from 'lodash/debounce';
import {clamp} from '@shopify/javascript-utilities/math';

import {hsbToRgb} from '../../utilities/color-transformers';
import {classNames} from '../../utilities/css';
import {hsbToRgb, hexToHsb} from '../../utilities/color-transformers';
import type {HSBColor, HSBAColor} from '../../utilities/color-types';

import {AlphaPicker, HuePicker, Slidable, SlidableProps} from './components';
import {EventListener} from '../EventListener';

import {
AlphaField,
AlphaPicker,
HuePicker,
Slidable,
TextPicker,
} from './components';
import styles from './ColorPicker.scss';

interface State {
pickerSize: number;
pickerWidth: number;
pickerHeight: number;
}

interface Position {
x: number;
y: number;
}

interface Color extends HSBColor {
Expand All @@ -29,36 +44,64 @@ export interface ColorPickerProps {

export class ColorPicker extends React.PureComponent<ColorPickerProps, State> {
state: State = {
pickerSize: 0,
pickerWidth: 0,
pickerHeight: 0,
};

private colorNode: HTMLElement | null = null;

private handleResize = debounce(
() => {
if (this.colorNode == null) return;
this.setState({
pickerWidth: this.colorNode.clientWidth,
pickerHeight: this.colorNode.clientHeight,
});
},
50,
{trailing: true},
);

componentDidMount() {
const {colorNode} = this;
if (colorNode == null) {
return;
}

this.setState({pickerSize: colorNode.clientWidth});
this.setState({
pickerWidth: colorNode.clientWidth,
pickerHeight: colorNode.clientHeight,
});

if (process.env.NODE_ENV === 'development') {
setTimeout(() => {
this.setState({pickerSize: colorNode.clientWidth});
this.setState({
pickerWidth: colorNode.clientWidth,
pickerHeight: colorNode.clientHeight,
});
}, 0);
}
}

render() {
const {id, color, allowAlpha} = this.props;
const {hue, saturation, brightness, alpha: providedAlpha} = color;
const {pickerSize} = this.state;
const {pickerWidth, pickerHeight} = this.state;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, do we need to update these values on resize?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, they should be updated 👍


const alpha = providedAlpha != null && allowAlpha ? providedAlpha : 1;
const {red, green, blue} = hsbToRgb({hue, saturation: 1, brightness: 1});
const colorString = `rgba(${red}, ${green}, ${blue}, ${alpha})`;
const draggerX = clamp(saturation * pickerSize, 0, pickerSize);
const draggerY = clamp(pickerSize - brightness * pickerSize, 0, pickerSize);
const draggerX = clamp(saturation * pickerWidth, 0, pickerWidth);
const draggerY = clamp(
pickerHeight - brightness * pickerHeight,
0,
pickerHeight,
);

const className = classNames(
styles.MainColor,
allowAlpha && styles.AlphaAllowed,
);

const alphaSliderMarkup = allowAlpha ? (
<AlphaPicker
Expand All @@ -68,25 +111,44 @@ export class ColorPicker extends React.PureComponent<ColorPickerProps, State> {
/>
) : null;

const hexPickerMarkup = (
<TextPicker
color={color}
allowAlpha={allowAlpha}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having to pass an allowAlpha, which the TextPicker shouldn't really care about imo. Could we render them in a grid or even a flex container and let this component take a bit more space than the alpha field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue with not passing it is the use of allowAlpha in the TextFieldSwatch.

onChange={this.handleHexChange}
/>
);

const alphaFieldMarkup = allowAlpha ? (
<AlphaField alpha={alpha} onChange={this.handleAlphaChange} />
) : null;

return (
<div
className={styles.ColorPicker}
id={id}
onMouseDown={this.handlePickerDrag}
>
<div ref={this.setColorNode} className={styles.MainColor}>
<div
className={styles.ColorLayer}
style={{backgroundColor: colorString}}
/>
<Slidable
onChange={this.handleDraggerMove}
draggerX={draggerX}
draggerY={draggerY}
/>
<div>
<div
className={styles.ColorPicker}
id={id}
onMouseDown={this.handlePickerDrag}
>
<div ref={this.setColorNode} className={className}>
<div
className={styles.ColorLayer}
style={{backgroundColor: colorString}}
/>
<Slidable
onChange={this.handleDraggerMove}
draggerX={draggerX}
draggerY={draggerY}
/>
</div>
<HuePicker hue={hue} onChange={this.handleHueChange} />
{alphaSliderMarkup}
</div>
<HuePicker hue={hue} onChange={this.handleHueChange} />
{alphaSliderMarkup}
<div className={styles.TextFields}>
{hexPickerMarkup}
{alphaFieldMarkup}
</div>
<EventListener event="resize" handler={this.handleResize} />
</div>
);
}
Expand All @@ -111,15 +173,24 @@ export class ColorPicker extends React.PureComponent<ColorPickerProps, State> {
onChange({hue, brightness, saturation, alpha});
};

private handleDraggerMove: SlidableProps['onChange'] = ({x, y}) => {
const {pickerSize} = this.state;
private handleHexChange = (hex: string) => {
const {
color: {alpha = 1},
onChange,
} = this.props;
const newColor = hexToHsb(hex);
onChange({...newColor, alpha});
};
Comment on lines +176 to +183
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we prevent invalid hex codes from being added, or maybe our error message could include why it's invalid? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was discussed over in the Figma designs. We had error messages implemented first, but then decided to change to prevent invalid hex values being entered. The check is made in handleBlur in the TextPicker component, by using the last valid value if an invalid value is entered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, so if a user enters #1234567 they'll have to enter the code again rather than deleting the extra character?

Copy link
Contributor Author

@ruairiphackett ruairiphackett Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, so if a user enters #1234567 they'll have to enter the code again rather than deleting the extra character?

That is a good point @AndrewMusgrave any thoughts on this scenario @mirualves?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave this up to Miru's best judgment 😄


private handleDraggerMove = ({x, y}: Position) => {
const {pickerWidth, pickerHeight} = this.state;
const {
color: {hue, alpha = 1},
onChange,
} = this.props;

const saturation = clamp(x / pickerSize, 0, 1);
const brightness = clamp(1 - y / pickerSize, 0, 1);
const saturation = clamp(x / pickerWidth, 0, 1);
const brightness = clamp(1 - y / pickerHeight, 0, 1);

onChange({hue, saturation, brightness, alpha});
};
Expand Down
62 changes: 62 additions & 0 deletions src/components/ColorPicker/components/AlphaField/AlphaField.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import React, {useCallback, useEffect, useState} from 'react';
import {clamp} from '@shopify/javascript-utilities/math';

import {useI18n} from '../../../../utilities/i18n';
import {TextField} from '../../../TextField';
import styles from '../../ColorPicker.scss';

export interface AlphaFieldProps {
alpha: number;
onChange(alpha: number): void;
}

export function AlphaField({alpha, onChange}: AlphaFieldProps) {
const i18n = useI18n();

const [percentage, setPercentage] = useState(
clamp(Math.round(alpha * 100) || 0, 0, 100),
);

const label = i18n.translate(
'Polaris.ColorPicker.alphaFieldAccessibilityLabel',
);

useEffect(() => {
setPercentage(Math.round(alpha * 100));
}, [alpha]);

const handleTextChange = useCallback((value) => {
setPercentage(value);
}, []);

const handleBlur = useCallback(() => {
const normalizedPercentage = clamp(percentage, 0, 100);

if (normalizedPercentage !== null) {
setPercentage(normalizedPercentage);

const alphaHasChanged = normalizedPercentage !== alpha * 100;

if (alphaHasChanged) {
onChange(normalizedPercentage / 100);
}
}
}, [alpha, onChange, percentage]);

return (
<div className={styles.AlphaField}>
<TextField
suffix="%"
value={percentage.toString()}
label={label}
labelHidden
type="number"
autoComplete={false}
max={100}
min={0}
onChange={handleTextChange}
onBlur={handleBlur}
/>
</div>
);
}
2 changes: 2 additions & 0 deletions src/components/ColorPicker/components/AlphaField/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export {AlphaField} from './AlphaField';
export type {AlphaFieldProps} from './AlphaField';
Loading