-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
DuotonePicker, DuotoneSwatch: Convert to TypeScript #49060
Changes from all commits
a1a31bc
66f19f4
4ce6ed1
6bbbb04
e20e675
5b39e04
d4e8260
11295e1
77b648c
6ee7027
ea8ce4f
f786ff3
2c31a5c
e3f041c
8142875
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
export type DuotonePickerProps = { | ||
/** | ||
* Whether there should be a button to clear the duotone value. | ||
* | ||
* @default true | ||
*/ | ||
clearable?: boolean; | ||
/** | ||
* Whether there should be an `unset` option. | ||
* | ||
* @default true | ||
*/ | ||
unsetable?: boolean; | ||
/** | ||
* Array of color presets of the form `{ color: '#000000', name: 'Black', slug: 'black' }`. | ||
*/ | ||
colorPalette: Color[]; | ||
/** | ||
* Array of duotone presets of the form `{ colors: [ '#000000', '#ffffff' ], name: 'Grayscale', slug: 'grayscale' }`. | ||
*/ | ||
duotonePalette: DuotoneColor[]; | ||
/** | ||
* Whether custom colors should be disabled. | ||
* | ||
* @default false | ||
*/ | ||
disableCustomColors?: boolean; | ||
/** | ||
* Whether custom duotone values should be disabled. | ||
* | ||
* @default false | ||
*/ | ||
disableCustomDuotone?: boolean; | ||
/** | ||
* An array of colors for the duotone effect. | ||
*/ | ||
value?: string[] | 'unset'; | ||
/** | ||
* Callback which is called when the duotone colors change. | ||
*/ | ||
onChange: ( value: DuotonePickerProps[ 'value' ] | undefined ) => void; | ||
}; | ||
|
||
type Color = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Not required for this PR, so feel free to ignore) As we work on color-related components, it would be great to take a wider look at these components' types, and see if we can better highlight dependencies and/or find inconsisntencies. For example, the |
||
color: string; | ||
name: string; | ||
slug: string; | ||
}; | ||
|
||
type DuotoneColor = { | ||
colors: string[]; | ||
name: string; | ||
slug: string; | ||
}; | ||
|
||
export type DuotoneSwatchProps = { | ||
/** | ||
* An array of colors to show or `null` to show the placeholder swatch icon. | ||
*/ | ||
values?: string[] | null; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,11 @@ | |
import { colord, extend } from 'colord'; | ||
import namesPlugin from 'colord/plugins/names'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import type { DuotonePickerProps } from './types'; | ||
|
||
extend( [ namesPlugin ] ); | ||
|
||
/** | ||
|
@@ -18,11 +23,13 @@ extend( [ namesPlugin ] ); | |
/** | ||
* Calculate the brightest and darkest values from a color palette. | ||
* | ||
* @param {Object[]} palette Color palette for the theme. | ||
* @param palette Color palette for the theme. | ||
* | ||
* @return {string[]} Tuple of the darkest color and brightest color. | ||
* @return Tuple of the darkest color and brightest color. | ||
*/ | ||
export function getDefaultColors( palette ) { | ||
export function getDefaultColors( | ||
palette: DuotonePickerProps[ 'colorPalette' ] | ||
) { | ||
// A default dark and light color are required. | ||
if ( ! palette || palette.length < 2 ) return [ '#000', '#fff' ]; | ||
|
||
|
@@ -38,20 +45,26 @@ export function getDefaultColors( palette ) { | |
current.brightness >= max.brightness ? current : max, | ||
]; | ||
}, | ||
[ { brightness: 1 }, { brightness: 0 } ] | ||
[ | ||
{ brightness: 1, color: '' }, | ||
{ brightness: 0, color: '' }, | ||
] | ||
) | ||
.map( ( { color } ) => color ); | ||
} | ||
Comment on lines
+48
to
54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An interesting example of how some clever functional code may not play nice with TS 😆 The initial values ( I think this is probably the least annoying/invasive way around it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agreed! Although annoying at times, I'm glad that TS is able to pick up these edge cases. The fact that the author of these lines of code was clever in writing the original implementation doesn't necessarily mean that a malformed color palette (e.g with out of scale brightness values) or another developer making amends to the algorithm could introduce a bug later in time. TS checks are great in avoiding such events There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though for this particular function I would rely on some unit tests more than TS 🫣 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's true! I guess in my mind I implicitly considered TS static linting as a the first test that runs against the code. |
||
|
||
/** | ||
* Generate a duotone gradient from a list of colors. | ||
* | ||
* @param {string[]} colors CSS color strings. | ||
* @param {string} angle CSS gradient angle. | ||
* @param colors CSS color strings. | ||
* @param angle CSS gradient angle. | ||
* | ||
* @return {string} CSS gradient string for the duotone swatch. | ||
* @return CSS gradient string for the duotone swatch. | ||
*/ | ||
export function getGradientFromCSSColors( colors = [], angle = '90deg' ) { | ||
export function getGradientFromCSSColors( | ||
colors: string[] = [], | ||
angle = '90deg' | ||
) { | ||
const l = 100 / colors.length; | ||
|
||
const stops = colors | ||
|
@@ -64,11 +77,11 @@ export function getGradientFromCSSColors( colors = [], angle = '90deg' ) { | |
/** | ||
* Convert a color array to an array of color stops. | ||
* | ||
* @param {string[]} colors CSS colors array | ||
* @param colors CSS colors array | ||
* | ||
* @return {Object[]} Color stop information. | ||
* @return Color stop information. | ||
*/ | ||
export function getColorStopsFromColors( colors ) { | ||
export function getColorStopsFromColors( colors: string[] ) { | ||
return colors.map( ( color, i ) => ( { | ||
position: ( i * 100 ) / ( colors.length - 1 ), | ||
color, | ||
|
@@ -78,10 +91,12 @@ export function getColorStopsFromColors( colors ) { | |
/** | ||
* Convert a color stop array to an array colors. | ||
* | ||
* @param {Object[]} colorStops Color stop information. | ||
* @param colorStops Color stop information. | ||
* | ||
* @return {string[]} CSS colors array. | ||
* @return CSS colors array. | ||
*/ | ||
export function getColorsFromColorStops( colorStops = [] ) { | ||
export function getColorsFromColorStops( | ||
colorStops: { position: number; color: string }[] = [] | ||
mirka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) { | ||
return colorStops.map( ( { color } ) => color ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered whether it was worth doing a tuple (
[ string, string ]
) here instead ofstring[]
, but decided against it for two reasons:CustomGradientPicker
acceptsstring[]
, so we'll need to do some added type massaging there.[ '#foo', '#bar' ] as const
to make the type checks pass, which is non-obvious.Does that sound reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tuple would be definitely better in representing the actual type that the component is expecting — without it, TS wouldn't be able to detect a malformed
value
(ie.[]
,[ '#fff' ]
etc).But the points that you make are valid, and therefore I'd be ok with typing it as
string[]
, at least initially. We can always narrow the type (or put more runtime checks in place) later as we see fitThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you pointed out recently, the new const type improvements may come handy for this scenario!