feat: wrap spectrum View, Text and Heading to accept custom colors#1903
feat: wrap spectrum View, Text and Heading to accept custom colors#1903
Conversation
- Exposes our extra semantic colors to these components - Created colorUtils in theme, containing theme variables - Move spectrumUtils to spectrum/utils (was originally going to put it here) - Added eslint config to allow using UNSAFE_styles/UNSAFE_classname
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1903 +/- ##
==========================================
+ Coverage 46.20% 46.23% +0.03%
==========================================
Files 641 645 +4
Lines 38218 38241 +23
Branches 9670 9672 +2
==========================================
+ Hits 17659 17682 +23
Misses 20506 20506
Partials 53 53
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
General idea looks good. I think you may need to memoize the converted props since the I think you could incorporate a general util here. Something like: export function wrapColorProps<
TProps extends Record<string, unknown> & {
backgroundColor?: ColorValue;
color?: ColorValue;
UNSAFE_style?: React.CSSProperties;
},
>(props: TProps): Omit<TProps, 'backgroundColor' | 'color'> {
const { backgroundColor, color, ...rest } = props;
if (backgroundColor == null && color == null) {
return props;
}
return {
...rest,
UNSAFE_style: {
...props.UNSAFE_style,
backgroundColor:
backgroundColor == null ? undefined : colorValueStyle(backgroundColor),
color: color == null ? undefined : colorValueStyle(color),
},
};
}And then consuming it could be like: const colorProps = useMemo(() => wrapColorProps(props), [props]);
return <SpectrumView {...colorProps} />;Alternatively, we could make a custom hook like |
bmingles
left a comment
There was a problem hiding this comment.
Left some comments. The suggestion for wrapColorProps function may impact the other suggestions, so I would look at that comment first to see what you think.
|
Kept it simple. Just did a useMemo for style, and allowed colorValueStyle to return undefined. Didn't seem worth a separate abstraction. |
|
I didn't do an exhaustive search, but replaced some usages. We should add a |
Tested with: