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.

Sign up for GitHub

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

Enhance the new color picker design #34598

Merged
merged 2 commits into from
Sep 22, 2021
Merged

Enhance the new color picker design #34598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fe0eca
Apply suggestions from code review
jorgefilipecosta Sep 3, 2021
430b261
Apply suggestions from code review
jorgefilipecosta Sep 9, 2021
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
30 changes: 16 additions & 14 deletions packages/components/src/range-control/styles/range-control-styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@ import NumberControl from '../../number-control';
import { COLORS, reduceMotion, rtl } from '../../utils';
import { space } from '../../ui/utils/space';

const rangeHeight = () => css( { height: 30, minHeight: 30 } );
const thumbSize = 12;
const rangeHeightValue = 30;
const railHeight = 4;
const rangeHeight = () =>
css( { height: rangeHeightValue, minHeight: rangeHeightValue } );
const thumbSize = 9;

export const Root = styled.div`
-webkit-tap-highlight-color: transparent;
Expand All @@ -38,7 +41,6 @@ export const Wrapper = styled.div`
color: ${ COLORS.blue.medium.focus };
display: block;
flex: 1;
padding-top: 18px;
position: relative;
width: 100%;

Expand All @@ -48,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 } ) }
`;
Expand All @@ -78,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 };
`;
Expand All @@ -101,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 };
Expand Down Expand Up @@ -136,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;
Expand Down Expand Up @@ -179,7 +181,7 @@ export const ThumbWrapper = styled.span`
display: flex;
height: ${ thumbSize }px;
justify-content: center;
margin-top: 9px;
margin-top: ${ ( rangeHeightValue - thumbSize ) / 2 }px;
outline: 0;
pointer-events: none;
position: absolute;
Expand Down Expand Up @@ -261,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;
Expand Down
7 changes: 6 additions & 1 deletion packages/components/src/ui/color-picker/color-display.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
77 changes: 42 additions & 35 deletions packages/components/src/ui/color-picker/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import type { ColorFormats } from 'tinycolor2';
* WordPress dependencies
*/
import { useState } from '@wordpress/element';
import { moreVertical } from '@wordpress/icons';
import { settings } from '@wordpress/icons';
import { useDebounce } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';

Expand All @@ -22,9 +22,13 @@ import {
WordPressComponentProps,
} from '../context';
import { HStack } from '../../h-stack';
import Button from '../../button';
import { Spacer } from '../../spacer';
import { ColorfulWrapper, SelectControl } from './styles';
import {
ColorfulWrapper,
SelectControl,
AuxiliaryColorArtefactWrapper,
DetailsControlButton,
} from './styles';
import { ColorDisplay } from './color-display';
import { ColorInput } from './color-input';
import { Picker } from './picker';
Expand Down Expand Up @@ -95,44 +99,47 @@ const ColorPicker = (
color={ safeColor }
enableAlpha={ enableAlpha }
/>
<HStack justify="space-between">
{ showInputs ? (
<SelectControl
options={ options }
value={ colorType }
onChange={ ( nextColorType ) =>
setColorType( nextColorType as ColorType )
<AuxiliaryColorArtefactWrapper>
<HStack justify="space-between">
{ 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
</HStack>
<Spacer margin={ 4 } />
{ showInputs && (
<ColorInput
colorType={ colorType }
color={ safeColor }
colorType={ copyFormat || colorType }
onChange={ handleChange }
enableAlpha={ enableAlpha }
/>
) }
<Button
onClick={ () => setShowInputs( ! showInputs ) }
icon={ moreVertical }
isPressed={ showInputs }
label={
showInputs
? __( 'Hide detailed inputs' )
: __( 'Show detailed inputs' )
}
/>
</HStack>
<Spacer />
{ showInputs && (
<ColorInput
colorType={ colorType }
color={ safeColor }
onChange={ handleChange }
enableAlpha={ enableAlpha }
/>
) }
</AuxiliaryColorArtefactWrapper>
</ColorfulWrapper>
);
};
Expand Down
12 changes: 8 additions & 4 deletions packages/components/src/ui/color-picker/hex-input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -35,10 +35,14 @@ export const HexInput = ( { color, onChange, enableAlpha }: HexInputProps ) => {
: colorized.toHexString();

return (
<InputControl
__unstableInputWidth="8em"
<ColorHexInputControl
prefix={
<Spacer as={ Text } marginLeft={ space( 2 ) } color="blue">
<Spacer
as={ Text }
marginLeft={ space( 3.5 ) }
color="blue"
lineHeight={ 1 }
>
#
</Spacer>
}
Expand Down
15 changes: 9 additions & 6 deletions packages/components/src/ui/color-picker/input-with-slider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
/**
* Internal dependencies
*/
import NumberControl from '../../number-control';
import { HStack } from '../../h-stack';
import { Text } from '../../text';
import { Spacer } from '../../spacer';
import { space } from '../utils/space';
import { RangeControl } from './styles';
import { RangeControl, NumberControlWrapper } from './styles';

interface InputWithSliderProps {
min: number;
Expand All @@ -26,17 +25,21 @@ export const InputWithSlider = ( {
value,
}: InputWithSliderProps ) => {
return (
<Spacer as={ HStack }>
<NumberControl
__unstableInputWidth="5em"
<Spacer as={ HStack } spacing={ 4 }>
<NumberControlWrapper
min={ min }
max={ max }
label={ label }
hideLabelFromVision
value={ value }
onChange={ onChange }
prefix={
<Spacer as={ Text } paddingLeft={ space( 1 ) } color="blue">
<Spacer
as={ Text }
paddingLeft={ space( 3.5 ) }
color="blue"
lineHeight={ 1 }
>
{ abbreviation }
</Spacer>
}
Expand Down
4 changes: 3 additions & 1 deletion packages/components/src/ui/color-picker/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
);
Expand Down
55 changes: 55 additions & 0 deletions packages/components/src/ui/color-picker/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,32 @@ import styled from '@emotion/styled';
/**
* Internal dependencies
*/
import NumberControl from '../../number-control';
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 } {
width: ${ space( 24 ) };
}
`;

export const SelectControl = styled( InnerSelectControl )`
margin-left: ${ space( -2 ) };
width: 5em;
${ BackdropUI } {
display: none;
}
`;

export const RangeControl = styled( InnerRangeControl )`
Expand All @@ -23,6 +42,25 @@ export const RangeControl = styled( InnerRangeControl )`
}
`;

// All inputs should be the same height so this should be changed at the component level.
// That involves changing heights of multiple input types probably buttons too etc.
// So until that is done we are already using the new height on the color picker so it matches the mockups.
Comment on lines +45 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please open a new issue for this task, and add it to tracking issue #34284?

const inputHeightStyle = `
&&& ${ Input } {
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 ) };
`;

export const ColorfulWrapper = styled.div`
width: 216px;

Expand Down Expand Up @@ -53,9 +91,26 @@ export const ColorfulWrapper = styled.div`
.react-colorful__pointer {
height: 16px;
width: 16px;
border: ${ CONFIG.borderWidthFocus } solid rgba( 255, 255, 255, 0 );
box-shadow: inset 0px 0px 0px ${ CONFIG.borderWidthFocus } #ffffff;
Comment on lines +94 to +95
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably use the box-shadow technique for borders, as suggested by @jasmussen in #34712 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this technique works for this case given that we need border and box-shadow at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can stack box shadows, that's what I did in 2997a2f.

}

${ interactiveHueStyles }

${ StyledField } {
margin-bottom: 0;
}

${ inputHeightStyle }
`;

export const DetailsControlButton = styled( Button )`
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit unfortunate that this is not a button that can be shown properly with just props. Why do we need to tweak instead of relying on composition of uncustomized Button (is the isSmall prop not enough?)

Copy link
Member Author

Choose a reason for hiding this comment

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

The isSmall prop on buttons with just an icon creates a 36-pixel width per 24 pixels height. Here we want a 24x24 pixels button.

Copy link
Contributor

@youknowriad youknowriad Sep 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The isSmall prop on buttons with just an icon creates a 36-pixel width per 24 pixels height. Here we want a 24x24 pixels button.

Do you think we should fix it in the button's styles instead?

	&.is-small {
		height: 24px;
		line-height: 22px;
		padding: 0 8px;
		font-size: 11px;

		&.has-icon:not(.has-text) {
			padding: 0 8px;
			width: 24px;
		}
	}

According to this style from the button's styles, it's meant to be 24px maybe there's bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I will try to understand if it is a bug or not that the button appears with 36px, but the padding and other styles suggest it is intentional.

&&&& {
min-width: ${ space( 6 ) };
padding: 0;
}
`;

export const ColorHexInputControl = styled( InputControl )`
width: 8em;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I have similar questions for all the customized lower level components: InputControl, SelectControl, NumberControl... All of these get specific styles here. While in some cases it's fine, I do wonder whether our components don't compose well enough.

In other words, if we struggle to use the base components ourselves to build higher-level ones, it's most likely that the same struggles will exist for folks trying to use the components in their apps (or other wordpress packages like block-editor, edit-post...)

`;
2 changes: 1 addition & 1 deletion packages/components/src/ui/tooltip/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/ui/tooltip/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down