fix: view border styling#2063
Conversation
| // 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 |
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
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
|
Connected with deephaven/deephaven-plugins#506 so that the border color styling props are applied |
mofojed
left a comment
mofojed
left a comment
There was a problem hiding this comment.
It's fine as is, but would be incorrect for RTL languages. We may as well add that support now.
| const style = useMemo(() => { | ||
| const borderStyle: CSSProperties = {}; | ||
| if (borderColor !== undefined) { | ||
| borderStyle.borderColor = colorValueStyle(borderColor); |
There was a problem hiding this comment.
It would be nice if we could just pass our own conversion function into View... Right now View is using useStyleProps with the viewStyleProps handlers to convert functions: https://github.com/adobe/react-spectrum/blob/bd458c1ed166a5ff1fd93f0c1a397e1540b3d880/packages/%40react-spectrum/view/src/View.tsx#L26C43-L26C57
If we could just pass our own color conversion handler in there somehow, then we wouldn't have to rewrite these props for every color: https://github.com/adobe/react-spectrum/blob/bd458c1ed166a5ff1fd93f0c1a397e1540b3d880/packages/%40react-spectrum/utils/src/styleProps.ts#L263
I'm tempted to file a ticket/PR in the Adobe GitHub repo to add this functionality, since we're ending up having to duplicate a bunch of things here just to handle the borders... but this should be fine for now.
3 participants
Exposed border color style props for view component.
Had to define border color styling using shorthand properties instead to avoid re-rendering bugs. Left comment in code explaining why this approach was taken. Checked to see if border width styling also had similar bugs but weirdly enough it doesn't.