-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add ColorArea, ColorField, ColorSlider, and ColorWheel to RAC #6199
Conversation
gradientProps: { | ||
...gradientStyleProps, | ||
role: 'presentation' | ||
}, |
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.
Removed the need for an additional gradient element inside the color area. Now both gradients are applied to the root color area element.
colorAreaStyles = { | ||
background: bg.join(', ') | ||
}; | ||
break; | ||
} | ||
} |
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.
This refactors the way the gradients are generated to be both simpler and more correct. For RGB, we use background-blend-mode
to combine the channel values rather than a transparency mask. This produces a more correct result than before, where the pixels at a given x/y position are closer to the actual value you get when you click. Some differences may appear in Chromatic for this.
For HSL and HSB, transparency is still used for saturation and lightness/brightness. But we had some bugs especially in HSB that are now fixed. For example, in the hue/saturation configuration, when lowering the brightness, the result shown in the thumb is much grayer than shown in the gradient. Same in the hue/brightness configuration with the saturation lowered.
These are both now fixed.
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.
Ran chromatic, it didn't appear to pick up on these https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=737
onChange | ||
}), state, ref); | ||
let {inputProps, ...otherProps} = useFormattedTextField({ | ||
...props, |
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.
mergeProps
resulted in onChange
being called twice, with both the raw text value of the field, and with the parsed color. It should only be called with a 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 think this has closed #5484
@@ -109,6 +112,15 @@ export function useColorSlider(props: AriaColorSliderOptions, state: ColorSlider | |||
inputProps['aria-valuetext'] += `, ${value.getColorName(locale)}`; | |||
} | |||
|
|||
let {visuallyHiddenProps} = useVisuallyHidden({ |
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.
Building visually hidden into the hook for color slider and color wheel like we do for color area. Can't think of a reason why you wouldn't want it to be visually hidden.
@@ -59,13 +58,12 @@ function ColorArea(props: SpectrumColorAreaProps, ref: FocusableRef<HTMLDivEleme | |||
} | |||
ref={containerRef} | |||
style={{ | |||
...colorAreaProps.style, | |||
...(isDisabled ? {} : colorAreaProps.style), |
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.
Previously no gradient was returned by the hook when disabled, but I don't think we can assume that all color areas will work that way. This moves that logic into Spectrum instead.
isDisabled?: boolean | ||
} | ||
|
||
export const InternalColorThumbContext = createContext<InternalColorThumbContextValue | null>(null); |
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.
Ideally this context would be exported/not internal. However the API for the context is a bit abnormal in that it takes a lot of stuff that isn't just the same as the props, including props/refs for two hidden inputs. If we want to export it we would need to re-think this.
|
||
export const ColorWheelTrackContext = createContext<ContextValue<ColorWheelTrackContextValue, HTMLDivElement>>(null); | ||
|
||
function ColorWheelTrack(props: ColorWheelTrackProps, ref: ForwardedRef<HTMLDivElement>) { |
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.
Ended up creating a separate component for the ColorWheelTrack. I wanted the track to just be rendered as the background of the ColorWheel itself, but the clip-path we use makes this complicated. The thumb shouldn't actually be clipped, so the only way to do that is to make a separate sibling element. Couldn't reuse SliderTrack here because that expects a SliderState and exposes a few other render props that are specific to sliders.
@@ -132,11 +134,11 @@ export function useRenderProps<T>(props: RenderPropsHookOptions<T>) { | |||
|
|||
return { | |||
className: computedClassName ?? defaultClassName, | |||
style: computedStyle, | |||
children: computedChildren, | |||
style: (computedStyle || defaultStyle) ? {...defaultStyle, ...computedStyle} : undefined, |
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.
note that defaultStyle works slightly differently to defaultClassName and defaultChildren: it is merged with the computed style rather than being overridden. I think this makes sense otherwise whenever anyone used a style
render prop they would need to remember to merge the defaults manually. Since the merge is shallow, it is still possible to override/remove a style property from the defaults by setting it to undefined.
? props.style({...renderProps, defaultStyle}) | ||
: props.style; | ||
return {...defaultStyle, ...style}; | ||
}; |
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.
This merges the styles from context, defaultStyle, and local styles.
Build successful! 🎉 |
Build successful! 🎉 |
Should React-Spectrum ColorArea have a border like the ColorSlider and ColorWheel with |
onChange | ||
}), state, ref); | ||
let {inputProps, ...otherProps} = useFormattedTextField({ | ||
...props, |
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 think this has closed #5484
colorAreaStyles = { | ||
background: bg.join(', ') | ||
}; | ||
break; | ||
} | ||
} |
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.
Ran chromatic, it didn't appear to pick up on these https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=737
} | ||
|
||
/** | ||
* A color area allows users to adjust two channels of an RGB, HSL or HSB color value against a two-dimensional gradient background. |
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.
Are those just the color spaces we support? or is that actually all which use a square? I could've sworn LAB could use a square as well
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.
yeah we only support those right now. I'd like to have some kind of plugin system for them so we can add more in the future though.
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.
touch-friendly color areas that can be styled as needed. | ||
|
||
* **Customizable** – Support for adjusting two-channel values of an HSL, HSB or RGB color value. | ||
* **High quality interactions** – Mouse, touch, and keyboard input is supported via the [useMove](../react-aria/useMove.html) hook. Pressing the color area the thumb to that position. Text selection and touch scrolling are prevented while dragging. |
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.
* **High quality interactions** – Mouse, touch, and keyboard input is supported via the [useMove](../react-aria/useMove.html) hook. Pressing the color area the thumb to that position. Text selection and touch scrolling are prevented while dragging. | |
* **High quality interactions** – Mouse, touch, and keyboard input is supported via the [useMove](../react-aria/useMove.html) hook. Pressing the color area moves the thumb to that position. Text selection and touch scrolling are prevented while dragging. |
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.
Topic for further work, 2d color wheels and composing multiple areas
https://color.adobe.com/create/color-wheel
Maybe allow multiple color pickers to be stacked like this? we can't just layer them on top of each other with transparent backgrounds because the highest one will intercept the events and position only its thumb to wherever you clicked. Maybe we want a "headless" thumb mode down the road?
|
||
```tsx example | ||
function Example() { | ||
let [color, setColor] = React.useState(parseColor('#ff00ff')); |
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.
this is hex but the title says rgba, not a big deal, though maybe we should wait to use the hex until we add support for the alpha channel? #3302
instead we can use the rgba(r, g, b, a)
syntax here
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.
This is ColorSlider not ColorField. 🤔 And it is RGBA because there are sliders for all 4 channels.
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.
yeah, but we're only sending in a color with three channels, I know it'll default to 1 for opacity, but maybe misleading name? or misleading value being input? all the other examples include it
|
||
</details> | ||
|
||
## Features |
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.
no example for Creating a color picker
?
maybe we should link to that section?
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.
Link where? That section doesn't exist in the RSP or hook docs either...
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.
ah, sorry, i meant link to the ColorArea section titled that
Agreed, we should do this regardless
Can't users disable the background style entirely by using the merge styles to replace it with a null value? Then their css wouldn't need |
Build successful! 🎉 |
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.
Should ColorWheelTrack be included with the ColorThumb as a child of ColorWheel?
Build successful! 🎉 |
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.
LGTM, tested the stories and docs and verified the functionality + bugs from last testing session don't exist anymore. With regard to defaultClassName/defaultStyle/defaultChildren, I think it is fine to expose it on all components, albeit the lack of any actual defaultStyle/defaultChildren on most components might be a bit confusing to users, at the moment it doesn't really feel very obvious when a component actually has values provided for those.
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.
Add --spectrum-colorarea-border-color: ButtonBorder;
for consistency with forced-colors: active
.
We may also want to update the custom swatches in the Storybook stories for ColorArea and the controlled examples of ColorField and ColorWheel to use forced-color-adjust: none
.
|
||
.spectrum-ColorArea-gradient { | ||
display: none; | ||
} | ||
} | ||
} | ||
|
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.
Add --spectrum-colorarea-border-color: ButtonBorder;
to forced-colors: active
style declaration at:
react-spectrum/packages/@adobe/spectrum-css-temp/components/colorarea/skin.css
Lines 21 to 23 in 49ba0e7
.spectrum-ColorArea { | |
--spectrum-colorarea-fill-color-disabled : GrayText; | |
} |
.spectrum-ColorArea {
--spectrum-colorarea-fill-color-disabled : GrayText;
--spectrum-colorarea-border-color: ButtonBorder;
}
Build successful! 🎉 |
I've replaced the custom swatches with real ones using |
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.
LGTM, verified the new forced color border around the RSP ColorArea
Build successful! 🎉 |
Build successful! 🎉 |
## API Changes
unknown top level export { type: 'any' } @react-aria/colorColorAreaAria ColorAreaAria {
colorAreaProps: DOMAttributes
- gradientProps: DOMAttributes
thumbProps: DOMAttributes
xInputProps: InputHTMLAttributes<HTMLInputElement>
yInputProps: InputHTMLAttributes<HTMLInputElement>
} it changed:
ColorFieldAria ColorFieldAria {
+ descriptionProps: DOMAttributes
+ errorMessageProps: DOMAttributes
inputProps: HTMLAttributes<HTMLInputElement>
labelProps: LabelHTMLAttributes<HTMLLabelElement>
} it changed:
@react-stately/colorColorSliderState ColorSliderState {
getDisplayColor: () => Color
+ isDragging: boolean
setValue: (string | Color) => void
value: Color
} it changed:
ColorWheelState ColorWheelState {
decrement: (number) => void
getDisplayColor: () => Color
getThumbPosition: (number) => {
x: number
y: number
}
hue: number
increment: (number) => void
+ isDisabled: boolean
isDragging: boolean
pageStep: number
setDragging: (boolean) => void
setHue: (number) => void
setValue: (string | Color) => void
step: number
value: Color
}
it changed:
Colorchanged by:
Color {
clone: () => Color
formatChannelValue: (ColorChannel, string) => string
+ getChannelFormatOptions: (ColorChannel) => Intl.NumberFormatOptions
getChannelName: (ColorChannel, string) => string
getChannelRange: (ColorChannel) => ColorChannelRange
getChannelValue: (ColorChannel) => number
getColorChannels: () => [ColorChannel, ColorChannel, ColorChannel]
getColorName: (string) => string
- getColorSpace: () => ColorFormat
+ getColorSpace: () => ColorSpace
getColorSpaceAxes: ({
xChannel?: ColorChannel
yChannel?: ColorChannel
}) => ColorAxes
toFormat: (ColorFormat) => Color
toHexInt: () => number
toString: (ColorFormat | 'css') => string
withChannelValue: (ColorChannel, number) => Color
}
it changed:
|
Fixes #5484
This adds React Aria Components for our existing color picker hooks, including ColorArea, ColorField, ColorSlider, and ColorWheel. It also fixes some bugs and simplifies the API, especially for ColorArea. See inline comments.
Because the color components often return inline styles, e.g. the background gradients, and users might need to override/combine these with their own, I've also added support for
defaultClassName
,defaultStyle
, anddefaultChildren
as render props across all components. This allows you to access the defaults in a render prop function, and modify/merge them with your own values. For example, in ColorSlider, one might want to have a checkerboard pattern in the background behind the gradient when partially transparent. This can be done by concatenatingdefaultStyle.background
with your own. You could also add extra wrappers around the default children of an element, or combine our default class name with a custom one instead of overriding.One problem with inline styles is that you need to use
!important
if you want to override them in regular CSS, for example, to override the background with a gray color when disabled. An alternative to the current approach would be to expose the background as a custom property (e.g.--color-slider-background
). Then the user would have to setbackground
themselves using that var, and could merge with the checkerboard or override. However, then it would be required to use that var and there would be no background by default, which might be unexpected/harder to use. Another point is that there are several other inline styles currently, and not sure we'd want to have a lot of custom properties for each of these. Opening this point for debate. I think exposingdefaultStyle
/defaultChildren
/defaultClassName
is useful independent of this though.