Swatch: Remove component in favor of ColorIndicator #43068
Merged
Conversation
This file contains 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
|
Size Change: +31 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
carolinan
approved these changes
Aug 9, 2022
ciampo
reviewed
Aug 9, 2022
Comment on lines
26
to
38
| icon={ <Swatch fill={ value } /> } | ||
| onClick={ () => setIsOpen( ( prev ) => ! prev ) } | ||
| > | ||
| { label } | ||
| <Flex justify="flex-start"> | ||
| { value ? ( | ||
| <ColorIndicator colorValue={ value } /> | ||
| ) : ( | ||
| <Icon icon={ swatch } /> | ||
| ) } | ||
| <FlexItem>{ label }</FlexItem> | ||
| </Flex> |
There was a problem hiding this comment.
Is there a reason why we didn't just replace the value of the icon prop? Something like
<Button icon={
value ? (
<ColorIndicator colorValue={ value } />
) : (
<Icon icon={ swatch } />
) }
/>There was a problem hiding this comment.
Since ColorIndicator allows additional props to be passed to the component (unlike Swatch), I notices that the markup got an extra size attribute.
<span class="component-color-indicator" size="24" style="background: rgb(252, 255, 65);"></span>The Button component runs the icon prop trough the <Icon /> component and adds this a prop. To prevent this, it made sense to me moving it to the children.
I'm open to suggestions if you have a better solution.
There was a problem hiding this comment.
A followup question. Should the swatch icon be used here at all? Looks like the common way to indicate that there is no color is a circle with a diagonal line.
cc @jameskoster
There was a problem hiding this comment.
Perhaps I am mistaken, but my understanding of the above is that we display an Icon when no color value is set? If that's the case I wonder why this isn't built into the ColorIndicator component? 🤔 The use of a dedicated icon feels a bit unnecessary.
There was a problem hiding this comment.
ColorIndicator is quite an old component whose purpose was (from what I gather) to literally display a color. It doesn't do anything fancier than that.
Some time ago I had written a couple of issues drafting a plan to overhaul the color-related components, but we never had the capacity to work on that (initial investigation, wider tracking issue).
Let's keep this issue focused on replacing Swatch, without adding more features to ColorIndicator for the time being (as that should be discussed in the wider context of the tracking issue linked above)
There was a problem hiding this comment.
(@walbo , I'll look deeper into your questions later on!)
There was a problem hiding this comment.
Alright, I had a look at the code. It turns out that technically Button's icon prop works better when passing directly an svg (or at most, an Icon component), and so I think that, for this specific case, we have 2 choices:
Option A: Usa the same approach as the one used in this PR (although I would use `HStack` instead of `Flex`, and I would add some styles to align the swatch icon and the color indicator better)
diff --git a/packages/components/src/color-list-picker/index.js b/packages/components/src/color-list-picker/index.js
index 904bd4fb09..9748a8ea3b 100644
--- a/packages/components/src/color-list-picker/index.js
+++ b/packages/components/src/color-list-picker/index.js
@@ -11,7 +11,7 @@ import Button from '../button';
import ColorPalette from '../color-palette';
import ColorIndicator from '../color-indicator';
import Icon from '../icon';
-import { Flex, FlexItem } from '../flex';
+import { HStack } from '../h-stack';
function ColorOption( {
label,
@@ -28,14 +28,17 @@ function ColorOption( {
className="components-color-list-picker__swatch-button"
onClick={ () => setIsOpen( ( prev ) => ! prev ) }
>
- <Flex justify="flex-start">
+ <HStack justify="flex-start" spacing={ 2 }>
{ value ? (
- <ColorIndicator colorValue={ value } />
+ <ColorIndicator
+ colorValue={ value }
+ className="components-color-list-picker__swatch-color"
+ />
) : (
<Icon icon={ swatch } />
) }
- <FlexItem>{ label }</FlexItem>
- </Flex>
+ <span>{ label }</span>
+ </HStack>
</Button>
{ isOpen && (
<ColorPalette
diff --git a/packages/components/src/color-list-picker/style.scss b/packages/components/src/color-list-picker/style.scss
index 75965d5612..341824aefe 100644
--- a/packages/components/src/color-list-picker/style.scss
+++ b/packages/components/src/color-list-picker/style.scss
@@ -8,5 +8,14 @@
}
.components-color-list-picker__swatch-button {
+ // Used to simulate styles as a .button.has-icon (which this component can't
+ // opt into, because it has to show more than a simple SVG as the "icon")
padding: 6px;
}
+
+.components-color-list-picker__swatch-color {
+ // Match the 24px size of the `swatch` icon (used when no color is set)
+ width: 18px;
+ height: 18px;
+ margin: 3px;
+}Option B: We could also ditch the `ColorIndicator` component altogether and just create an ad-hoc `SVG` element. If on one hand this option "ditches" a pre-existing component for an ad-hoc solution, on the other hand it allows us to use the `icon` prop of `Button` and avoid style overrides and custom flex layouts.
diff --git a/packages/components/src/color-list-picker/index.js b/packages/components/src/color-list-picker/index.js
index 904bd4fb09..aa76a64401 100644
--- a/packages/components/src/color-list-picker/index.js
+++ b/packages/components/src/color-list-picker/index.js
@@ -1,17 +1,15 @@
/**
* WordPress dependencies
*/
-import { useState } from '@wordpress/element';
+import { useState, useMemo } from '@wordpress/element';
import { swatch } from '@wordpress/icons';
+import { SVG, Circle } from '@wordpress/primitives';
/**
* Internal dependencies
*/
import Button from '../button';
import ColorPalette from '../color-palette';
-import ColorIndicator from '../color-indicator';
-import Icon from '../icon';
-import { Flex, FlexItem } from '../flex';
function ColorOption( {
label,
@@ -22,20 +20,35 @@ function ColorOption( {
onChange,
} ) {
const [ isOpen, setIsOpen ] = useState( false );
+
+ const buttonIcon = useMemo(
+ () =>
+ value ? (
+ // A simple circle of the same size as the `swatch` icon,
+ // filled with a color and with a light border.
+ <SVG xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24">
+ <Circle
+ cx="12"
+ cy="12"
+ r="9"
+ fill={ value }
+ stroke="rgba(0, 0, 0, 0.2)"
+ />
+ </SVG>
+ ) : (
+ swatch
+ ),
+ [ value ]
+ );
+
return (
<>
<Button
className="components-color-list-picker__swatch-button"
+ icon={ buttonIcon }
onClick={ () => setIsOpen( ( prev ) => ! prev ) }
>
- <Flex justify="flex-start">
- { value ? (
- <ColorIndicator colorValue={ value } />
- ) : (
- <Icon icon={ swatch } />
- ) }
- <FlexItem>{ label }</FlexItem>
- </Flex>
+ { label }
</Button>
{ isOpen && (
<ColorPalette
diff --git a/packages/components/src/color-list-picker/style.scss b/packages/components/src/color-list-picker/style.scss
index 75965d5612..232be723ed 100644
--- a/packages/components/src/color-list-picker/style.scss
+++ b/packages/components/src/color-list-picker/style.scss
@@ -6,7 +6,3 @@
.components-color-list-picker__color-picker {
margin: $grid-unit-10 0;
}
-
-.components-color-list-picker__swatch-button {
- padding: 6px;
-}Both options would also improve the UI of duotone, where currently there's a jump in the UI when a color is picked:
Kapture.2022-08-11.at.13.20.57.mp4
I'm personally liking the simplicity and the lack of style overrides of option B, what do folks think ?
There was a problem hiding this comment.
I don't have a strong opinion on the Button children vs. icon prop part per se, but I do think it would be beneficial to keep using the ColorIndicator component. It would help us maintain consistency once we add the "no color" support. Another thing is size consistency. Joen wants us to update the swatches in ColorPalette so they're at the consistent 20px size, so we'll want to replace that implementation with ColorIndicator as well.
tl;dr — it's probably beneficial to use ColorIndicator everywhere.
There was a problem hiding this comment.
I do like the simplicity of the SVG workaround, but as @mirka mentions I think we should go with ColorIndicator to keep consistency accross components and not introduce different ad-hoc solutions.
I'll update the branch with the suggested changes, and align the icon and color indicator size.
There was a problem hiding this comment.
Updated the branch to use HStack and the fix to avoid jumping text. Tweaked the margin so the ColorIndicator is still 20px instead of 18px as you suggested @ciampo
Skjermopptak.2022-08-11.kl.17.17.28.mov
Co-Authored-By: Marco Ciampini <[email protected]>
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
Use
ColorIdendicatorinstead ofSwatchcomponent, and remove the now unusedSwatchcomponent. TheSwatchcomponent was never exported so removing it should not break any BC.Why?
SwatchandColorIndicatorare almost idendical and we don't need both components.How?
Swatchwas only used internally two places inside the components package (inColorListPickerandDuodoneSwatch). Replace the instances withColorIndenticator.Testing Instructions
Make sure
ColorListPickerandDuodoneSwatchworks the same as before.Screenshots or screencast