Conversation
ciampo
added
[Type] Code Quality
ciampo
requested review from
aaronrobertshaw,
jorgefilipecosta,
kienstra,
mirka,
t-hamano and
tyxla
ciampo
commented
Jan 24, 2023
| colorValue: CSSProperties[ 'borderColor' ], | ||
| colors: ColorProps[ 'colors' ] | undefined | ||
| ) => { | ||
| if ( ! colorValue || ! colors || colors.length === 0 ) { |
Contributor
Author
There was a problem hiding this comment.
This check is now embedded in isMultiplePaletteArray
ciampo
commented
Jan 24, 2023
| // Multiple origins | ||
| let matchedColor; | ||
|
|
||
| ( colors as PaletteObject[] ).some( ( origin ) => |
Contributor
Author
There was a problem hiding this comment.
Since isMultiplePaletteArray acts as a type guard, the cast to PaletteObject[] and to ColorObject[] isn't necessary anymore
ciampo
commented
Jan 24, 2023
Comment on lines
-268
to
-279
| // Make sure that the `colors` array has a valid format. | ||
| if ( | ||
| colors.length > 0 && | ||
| hasMultipleColorOrigins !== areColorsMultiplePalette( colors ) | ||
| ) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| 'wp.components.ColorPalette: please specify a valid format for the `colors` prop. ' | ||
| ); | ||
| return null; | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
This check isn't necessary anymore, because its goal was to check that the colors array and the __experimentalHasMultipleOrigins were in sync. Now that the __experimentalHasMultipleOrigins has been removed, we can safely delete the check
ciampo
commented
Jan 24, 2023
| * @default [] | ||
| */ | ||
| colors?: ( PaletteObject | ColorObject )[]; | ||
| colors?: PaletteObject[] | ColorObject[]; |
Contributor
Author
There was a problem hiding this comment.
This change is to reflect that the colors array cannot contain mixed PaletteObject and ColorObject items
ciampo
commented
Jan 24, 2023
Comment on lines
+59
to
+73
| // The PaletteObject type has a `colors` property (an array of ColorObject), | ||
| // while the ColorObject type has a `color` property (the CSS color value). | ||
| export const isMultiplePaletteObject = ( | ||
| obj: PaletteObject | ColorObject | ||
| ): obj is PaletteObject => | ||
| Array.isArray( ( obj as PaletteObject ).colors ) && ! ( 'color' in obj ); | ||
|
|
||
| export const isMultiplePaletteArray = ( | ||
| arr: ( PaletteObject | ColorObject )[] | ||
| ): arr is PaletteObject[] => { | ||
| return ( | ||
| arr.length > 0 && | ||
| arr.every( ( colorObj ) => isMultiplePaletteObject( colorObj ) ) | ||
| ); | ||
| }; |
Contributor
Author
There was a problem hiding this comment.
This is the only part that changes in this file. The rest of the lines are a simple copy/paste from index.tsx
ciampo
commented
Jan 24, 2023
Comment on lines
-108
to
+122
| const Component = | ||
| gradients?.length && gradients[ 0 ].gradients | ||
| ? MultipleOrigin | ||
| : SingleOrigin; | ||
| const Component = isMultipleOriginArray( gradients ) | ||
| ? MultipleOrigin | ||
| : SingleOrigin; |
Contributor
Author
There was a problem hiding this comment.
Applied the same logic used for ColorPalette also to GradientPicker, with the only difference that instead of checking for colors and color properties, we check for gradients and gradient
ciampo
commented
Jan 24, 2023
| */ | ||
| import type { ColorObject, ColorPaletteProps, PaletteObject } from './types'; | ||
|
|
||
| extend( [ namesPlugin, a11yPlugin ] ); |
Contributor
Author
There was a problem hiding this comment.
I had to add colord extensions here as well, since without it the unit tests would fail. I don't think that this should be problematic, but better to double-check too
|
Size Change: -22 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
aaronrobertshaw
approved these changes
Jan 25, 2023
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Great work @ciampo! ✨
These changes make sense and clean things up nicely. Thanks for all the inline comments providing context, they make reviewing a pleasure.
✅ Didn't spot any issues smoke testing various controls in both editors
✅ ColorPalette, GradientPicker & BorderControl components function as per trunk in Storybook
✅ All available unit tests for impacted components pass
✅ No typing errors
LGTM 🚢
…gin-types-and-utils
ciampo
deleted the
refactor/color-palette-tidy-up-multiple-origin-types-and-utils
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
As a follow-up to #46315, this PR further cleans up some of the logic around single vs multiple origin color palettes.
Why?
Code quality (re-use same checks in different components, add comments, tighten up types)
How?
Here's the main set of changes applies in this PR:
ColorPaletteutilities to a separateutilsfilelenghtcheck)ColorPalettethat isn't necessary anymore since the removal of the__experimentalHasMultipleOriginsprop in Remove: __experimentalHasMultipleOrigins prop from colors and gradients #46315ColorPalettelogic inBorderControlinstead of custom check (which allows to simplify some types and checks too!)GradientPickertooYou can review this PR commit-by-commit too
Testing Instructions
In Storybook, check that
ColorPalette,GradientPickerandBorderControlcomponents behave like ontrunk. Pay particular attention to testing these components with single vs multiple origin colors/gradients.Smoke test the block editor and make sure that the above components do not show any regressions.