Skip to content
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.

Sign up for GitHub

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

fix: view border styling #2063

Merged
merged 8 commits into from
Jun 14, 2024
Merged

fix: view border styling #2063

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4bf6031
fix: view border styling
ethanalvizo Jun 7, 2024
5e09674
comments
ethanalvizo Jun 7, 2024
4166c36
unit test fix
ethanalvizo Jun 7, 2024
abcd08d
PR comments
ethanalvizo Jun 7, 2024
f59b228
mor PR fixes
ethanalvizo Jun 7, 2024
1995e44
PR comments
ethanalvizo Jun 14, 2024
972c8da
linter
ethanalvizo Jun 14, 2024
98dec35
lint fix
ethanalvizo Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 42 additions & 2 deletions packages/components/src/spectrum/View.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ import { type ColorValue, colorValueStyle } from '../theme/colorUtils';

export type ViewProps = Omit<SpectrumViewProps<6>, 'backgroundColor'> & {
ethanalvizo marked this conversation as resolved.
Show resolved Hide resolved
backgroundColor?: ColorValue;
borderColor?: ColorValue;
borderStartColor?: ColorValue;
borderEndColor?: ColorValue;
borderTopColor?: ColorValue;
borderBottomColor?: ColorValue;
borderXColor?: ColorValue;
borderYColor?: ColorValue;
};

/**
Expand All @@ -23,13 +30,46 @@ export type ViewProps = Omit<SpectrumViewProps<6>, 'backgroundColor'> & {

ethanalvizo marked this conversation as resolved.
Show resolved Hide resolved
export const View = forwardRef<DOMRefValue<HTMLElement>, ViewProps>(
(props, forwardedRef): JSX.Element => {
const { backgroundColor, UNSAFE_style, ...rest } = props;
const {
backgroundColor,
borderColor,
borderStartColor,
borderEndColor,
borderTopColor,
borderBottomColor,
borderXColor,
borderYColor,
UNSAFE_style,
...rest
} = props;

// Using shorthand to define border styling. Removing a style property during rerender can lead to styling bugs
// ex. changing from borderXColor = blue to borderColor = blue will cause the left and right borders to disappear
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, I can't find the error you mentioned in the React spectrum source code at all. But the error also says "replace the shorthand with separate values", which is the opposite of what you're doing here (you're converting everything to shorthand).
Also, we shouldn't be setting these properties at all if they're not set; leave them undefined, not 'transparent'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad on this one I think I was misusing the useMemo hook earlier. The new way I define borderStyle defines everything into separate values and unset properties are no longer set to transparent

const defaultBorderColor = colorValueStyle(borderColor) ?? 'transparent';
const defaultBorderXColor =
colorValueStyle(borderXColor) ?? defaultBorderColor;
const defaultBorderYColor =
colorValueStyle(borderYColor) ?? defaultBorderColor;
const topColor = colorValueStyle(borderTopColor) ?? defaultBorderYColor;
const bottomColor =
colorValueStyle(borderBottomColor) ?? defaultBorderYColor;
const leftColor = colorValueStyle(borderStartColor) ?? defaultBorderXColor;
const rightColor = colorValueStyle(borderEndColor) ?? defaultBorderXColor;

const style = useMemo(
() => ({
...UNSAFE_style,
backgroundColor: colorValueStyle(backgroundColor),
borderColor: `${topColor} ${rightColor} ${bottomColor} ${leftColor}`,
}),
[backgroundColor, UNSAFE_style]
[
backgroundColor,
UNSAFE_style,
topColor,
rightColor,
bottomColor,
leftColor,
]
);

return <SpectrumView {...rest} ref={forwardedRef} UNSAFE_style={style} />;
Expand Down
Loading