Apply suggestions from code review

Co-authored-by: Marco Ciampini <[email protected]> (+1 squashed commit) Squashed commits: [eeb7fa834e] Implement new color picker design
WordPress · Sep 22, 2021 · 430b261 · 430b261
1 parent 0fe0eca
commit 430b261
Show file tree

Hide file tree

Showing 9 changed files with 83 additions and 49 deletions.
diff --git a/packages/components/src/range-control/styles/range-control-styles.js b/packages/components/src/range-control/styles/range-control-styles.js
@@ -13,6 +13,7 @@ import { COLORS, reduceMotion, rtl } from '../../utils';
 import { space } from '../../ui/utils/space';
 
 const rangeHeightValue = 30;
+const railHeight = 4;
 const rangeHeight = () =>
 	css( { height: rangeHeightValue, minHeight: rangeHeightValue } );
 const thumbSize = 9;
@@ -40,7 +41,6 @@ export const Wrapper = styled.div`
 	color: ${ COLORS.blue.medium.focus };
 	display: block;
 	flex: 1;
-	padding-top: 18px;
 	position: relative;
 	width: 100%;
 
@@ -50,13 +50,13 @@ export const Wrapper = styled.div`
 `;
 
 export const BeforeIconWrapper = styled.span`
-	margin-top: 3px;
+	margin-top: ${ railHeight }px;
 
 	${ rtl( { marginRight: 6 } ) }
 `;
 
 export const AfterIconWrapper = styled.span`
-	margin-top: 3px;
+	margin-top: ${ railHeight }px;
 
 	${ rtl( { marginLeft: 16 } ) }
 `;
@@ -80,11 +80,11 @@ export const Rail = styled.span`
 	pointer-events: none;
 	right: 0;
 	display: block;
-	height: 3px;
+	height: ${ railHeight }px;
 	position: absolute;
-	margin-top: 14px;
+	margin-top: ${ ( rangeHeightValue - railHeight ) / 2 }px;
 	top: 0;
-	border-radius: 9999px;
+	border-radius: ${ railHeight }px;
 
 	${ railBackgroundColor };
 `;
@@ -103,13 +103,13 @@ const trackBackgroundColor = ( { disabled, trackColor } ) => {
 
 export const Track = styled.span`
 	background-color: currentColor;
-	border-radius: 9999px;
+	border-radius: ${ railHeight }px;
 	box-sizing: border-box;
-	height: 3px;
+	height: ${ railHeight }px;
 	pointer-events: none;
 	display: block;
 	position: absolute;
-	margin-top: 14px;
+	margin-top: ${ ( rangeHeightValue - railHeight ) / 2 }px;
 	top: 0;
 
 	${ trackBackgroundColor };
@@ -138,7 +138,7 @@ const markFill = ( { disabled, isFilled } ) => {
 
 export const Mark = styled.span`
 	box-sizing: border-box;
-	height: 9px;
+	height: ${ thumbSize }px;
 	left: 0;
 	position: absolute;
 	top: -4px;
@@ -263,7 +263,7 @@ const tooltipPosition = ( { position } ) => {
 };
 
 export const Tooltip = styled.span`
-	background: ${ COLORS.ui.border };
+	background: rgba( 0, 0, 0, 0.8 );
 	border-radius: 2px;
 	box-sizing: border-box;
 	color: white;

diff --git a/packages/components/src/ui/color-picker/color-display.tsx b/packages/components/src/ui/color-picker/color-display.tsx
@@ -164,7 +164,12 @@ export const ColorDisplay = ( {
 				</Text>
 			}
 		>
-			<Flex justify="flex-start" gap={ space( 1 ) } ref={ copyRef }>
+			<Flex
+				justify="flex-start"
+				gap={ space( 1 ) }
+				ref={ copyRef }
+				style={ { height: 30 } }
+			>
 				<Component { ...props } />
 			</Flex>
 		</Tooltip>

diff --git a/packages/components/src/ui/color-picker/component.tsx b/packages/components/src/ui/color-picker/component.tsx
@@ -22,9 +22,13 @@ import {
 	WordPressComponentProps,
 } from '../context';
 import { HStack } from '../../h-stack';
-import Button from '../../button';
 import { Spacer } from '../../spacer';
-import { ColorfulWrapper, SelectControl, AuxiliaryColorArtefactWrapper } from './styles';
+import {
+	ColorfulWrapper,
+	SelectControl,
+	AuxiliaryColorArtefactWrapper,
+	DetailsControlButton,
+} from './styles';
 import { ColorDisplay } from './color-display';
 import { ColorInput } from './color-input';
 import { Picker } from './picker';
@@ -97,34 +101,35 @@ const ColorPicker = (
 			/>
 			<AuxiliaryColorArtefactWrapper>
 				<HStack justify="space-between">
-				{ showInputs ? (
-					<SelectControl
-						options={ options }
-						value={ colorType }
-						onChange={ ( nextColorType ) =>
-							setColorType( nextColorType as ColorType )
+					{ showInputs ? (
+						<SelectControl
+							options={ options }
+							value={ colorType }
+							onChange={ ( nextColorType ) =>
+								setColorType( nextColorType as ColorType )
+							}
+							label={ __( 'Color format' ) }
+							hideLabelFromVision
+						/>
+					) : (
+						<ColorDisplay
+							color={ safeColor }
+							colorType={ copyFormat || colorType }
+							enableAlpha={ enableAlpha }
+						/>
+					) }
+					<DetailsControlButton
+						isSmall
+						onClick={ () => setShowInputs( ! showInputs ) }
+						icon={ settings }
+						isPressed={ showInputs }
+						label={
+							showInputs
+								? __( 'Hide detailed inputs' )
+								: __( 'Show detailed inputs' )
 						}
-						label={ __( 'Color format' ) }
-						hideLabelFromVision
 					/>
-				) : (
-					<ColorDisplay
-						color={ safeColor }
-						colorType={ copyFormat || colorType }
-						enableAlpha={ enableAlpha }
-					/>
-				) }
-				<Button
-					onClick={ () => setShowInputs( ! showInputs ) }
-					icon={ settings }
-					isPressed={ showInputs }
-					label={
-						showInputs
-							? __( 'Hide detailed inputs' )
-							: __( 'Show detailed inputs' )
-					}
-				/>
-			</HStack>
+				</HStack>
 				<Spacer margin={ 4 } />
 				{ showInputs && (
 					<ColorInput

diff --git a/packages/components/src/ui/color-picker/hex-input.tsx b/packages/components/src/ui/color-picker/hex-input.tsx
@@ -13,8 +13,8 @@ import { __ } from '@wordpress/i18n';
  */
 import { Text } from '../../text';
 import { Spacer } from '../../spacer';
-import InputControl from '../../input-control';
 import { space } from '../utils/space';
+import { ColorHexInputControl } from './styles';
 
 interface HexInputProps {
 	color: ColorFormats.HSLA;
@@ -35,8 +35,7 @@ export const HexInput = ( { color, onChange, enableAlpha }: HexInputProps ) => {
 		: colorized.toHexString();
 
 	return (
-		<InputControl
-			__unstableInputWidth="8em"
+		<ColorHexInputControl
 			prefix={
 				<Spacer
 					as={ Text }

diff --git a/packages/components/src/ui/color-picker/input-with-slider.tsx b/packages/components/src/ui/color-picker/input-with-slider.tsx
@@ -25,7 +25,7 @@ export const InputWithSlider = ( {
 	value,
 }: InputWithSliderProps ) => {
 	return (
-		<Spacer as={ HStack }>
+		<Spacer as={ HStack } spacing={ 4 }>
 			<NumberControlWrapper
 				min={ min }
 				max={ max }

diff --git a/packages/components/src/ui/color-picker/stories/index.js b/packages/components/src/ui/color-picker/stories/index.js
@@ -48,7 +48,9 @@ const Example = () => {
 			marginTop={ space( 10 ) }
 		>
 			<ColorPicker { ...props } color={ color } onChange={ setColor } />
-			<div>{ colorize( color ).toHslString() }</div>
+			<div style={ { width: 200, textAlign: 'center' } }>
+				{ colorize( color ).toHslString() }
+			</div>
 			<ColorPicker { ...props } color={ color } onChange={ setColor } />
 		</Flex>
 	);

diff --git a/packages/components/src/ui/color-picker/styles.ts b/packages/components/src/ui/color-picker/styles.ts
@@ -11,11 +11,14 @@ import InnerSelectControl from '../../select-control';
 import InnerRangeControl from '../../range-control';
 import { StyledField } from '../../base-control/styles/base-control-styles';
 import { space } from '../utils/space';
+import Button from '../../button';
 import {
 	BackdropUI,
 	Container as InputControlContainer,
 	Input,
 } from '../../input-control/styles/input-control-styles';
+import InputControl from '../../input-control';
+import CONFIG from '../../utils/config-values';
 
 export const NumberControlWrapper = styled( NumberControl )`
 	${ InputControlContainer } {
@@ -47,6 +50,13 @@ const inputHeightStyle = `
 	height: 40px;
 }`;
 
+// Make the Hue circle picker not go out of the bar
+const interactiveHueStyles = `
+.react-colorful__interactive {
+	width: calc( 100% - ${ space( 2 ) } );
+	margin-left: ${ space( 1 ) };
+}`;
+
 export const AuxiliaryColorArtefactWrapper = styled.div`
 	padding: ${ space( 2 ) } ${ space( 4 ) };
 `;
@@ -81,13 +91,26 @@ export const ColorfulWrapper = styled.div`
 	.react-colorful__pointer {
 		height: 16px;
 		width: 16px;
-		border: 1.5px solid #ffffff;
-		box-shadow: 0px 0px 3px rgba( 0, 0, 0, 0.25 );
+		border: ${ CONFIG.borderWidthFocus } solid rgba( 255, 255, 255, 0 );
+		box-shadow: inset 0px 0px 0px ${ CONFIG.borderWidthFocus } #ffffff;
 	}
 
+	${ interactiveHueStyles }
+
 	${ StyledField } {
 		margin-bottom: 0;
 	}
 
 	${ inputHeightStyle }
 `;
+
+export const DetailsControlButton = styled( Button )`
+	&&&& {
+		min-width: ${ space( 6 ) };
+		padding: 0;
+	}
+`;
+
+export const ColorHexInputControl = styled( InputControl )`
+	width: 8em;
+`;
diff --git a/packages/components/src/ui/tooltip/styles.js b/packages/components/src/ui/tooltip/styles.js
@@ -28,7 +28,7 @@ export const TooltipContent = css`
 
 export const TooltipPopoverView = styled.div`
 	background: rgba( 0, 0, 0, 0.8 );
-	border-radius: 6px;
+	border-radius: 2px;
 	box-shadow: 0 0 0 1px rgba( 255, 255, 255, 0.04 );
 	color: ${ COLORS.white };
 	padding: 4px 8px;

diff --git a/packages/components/src/ui/tooltip/test/__snapshots__/index.js.snap b/packages/components/src/ui/tooltip/test/__snapshots__/index.js.snap
@@ -17,7 +17,7 @@ exports[`props should render correctly 1`] = `
 
 .emotion-2 {
   background: rgba( 0, 0, 0, 0.8 );
-  border-radius: 6px;
+  border-radius: 2px;
   box-shadow: 0 0 0 1px rgba( 255, 255, 255, 0.04 );
   color: #fff;
   padding: 4px 8px;