[fix] Handle undefined theme properties in CCv2 #13240
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ PR preview is ready!
|
There was a problem hiding this comment.
Pull request overview
This PR improves CSS custom property serialization in the BidiComponent theme utility to handle edge cases more robustly.
- Added special handling for
widgetBorderColorto use 'transparent' instead of 'unset' when null/undefined - Enhanced the serialization logic to properly handle null/undefined values, arrays, and unexpected types
- Added comprehensive unit tests to verify the new behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| frontend/lib/src/components/widgets/BidiComponent/utils/theme.ts | Enhanced objectToCssCustomProperties function with special case handling for widgetBorderColor, general null/undefined handling using 'unset', array serialization, and defensive fallback for unexpected types |
| frontend/lib/src/components/widgets/BidiComponent/utils/theme.test.ts | Added parameterized tests to verify special handling of null/undefined widgetBorderColor and general null/undefined property serialization |
There was a problem hiding this comment.
Question: In protoFieldsToIgnore (line 249), widgetBorderColor is listed - should we be passing this in CCv2 since it is a deprecated property?
There was a problem hiding this comment.
That's a good point!
The scenario we want to support is "if showWidgetBorders is false, then set this variable to transparent, else set this variable to the border color." Given what you said, would it make sense to expose this under a different name?
There was a problem hiding this comment.
My preference would be to expose this under a different name like widgetBorderStyle to distinguish, and because its more accurate to the property as a computed style value (not just a color).
Unfortunately we previously mixed concerns with the same name for a deprecated config (themeInput) property, widgetBorderColor that is just the color, and the computed theme output property theme.colors.widgetBorderColor which is a mix of a color and a boolean we use to determine whether widgets borders should be shown 😓 Once we update properties used by dependency teams off this deprecated one, can clean this up.
There was a problem hiding this comment.
Thank you. I took another look at this and given the constraints here, I think what makes the most sense is to add widgetBorderStyle as a fully computed shorthand (eg: it evaluates to 1px solid yellow). This will be generated using the theme values.
Why I think this makes more sense is that widgetBorderStyle as a name could give the impression that it's a fully declared shorthand, not just a standalone color. The other benefit is that it makes actually using this much simpler, now users just need to write border: var(--st-widget-border-style), and their borders will immediately match the rest of the app's theme.
I just pushed an update that implements this, WDYT about this approach?
There was a problem hiding this comment.
Agreed that widgetBorderStyle would probably indicate full specification, though I don't love how that would decrease flexibility for border styling 🤔
I played around with a couple options and I think we are better off just keeping widgetBorderColor (not deprecating this output). This would entail having it use "transparent" instead of undefined, removing from protoFieldsToIgnore, with documentation that this is the computed output NOT the deprecated config.
Apologies for the back and forth, trying to balance all considerations.
There was a problem hiding this comment.
I appreciate the thoroughness! Pushing an update that just keeps widgetBorderColor with the fallback to transparent.
2cc6f60 to
e7d4452
Compare
📈 Frontend coverage change detectedThe frontend unit test (vitest) coverage has increased by 0.0100%
✅ Coverage change is within normal range. |
frontend/lib/src/components/widgets/BidiComponent/utils/theme.ts
Outdated
Show resolved
Hide resolved
e7d4452 to
4f3b27e
Compare
Describe your changes
Improved CSS custom property serialization in the BidiComponent theme utility:
widgetBorderColorto use 'transparent' instead of 'unset' when the value is null or undefinedTesting Plan
widgetBorderColorand the general serialization of null/undefined values for both widget and non-widget properties.Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.