-
Notifications
You must be signed in to change notification settings - Fork 222
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
feat: Add style transform support for stencils #2489
feat: Add style transform support for stencils #2489
Conversation
Passing run #6646 ↗︎
Details:
Review all test suite changes for PR #2489 ↗︎ |
color: cssVar(brand.error.base, '#de2e21'), | ||
color: brand.error.base, |
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 type of change is in many places. Auto-fallback works and manually including a fallback is a maitenance issue.
… fix/style-transformer-stencil
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.
You write code, sometimes I agree with the code you write
|
||
let boxShadow: ts.TemplateExpression; | ||
switch (inset) { | ||
case 'outer': |
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.
Would outer
be considered to be both focus rings on the "outside" of the container? I noticed an inset
here or this something we need to talk about when re-factoring focusRing
?
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 why I think focusRing
will need an update. outer
is actually both inner:
canvas-kit/modules/react/common/lib/styles/focusRing.ts
Lines 10 to 13 in 8817a0a
* Specifies where the ring(s) should be attached. | |
* - undefined: Both "inner" and "outer" shadows outside the container. | |
* - 'inner': "Inner" shadow inset. "Outer" shadow outside the container. | |
* - 'outer': Both "inner" and "outer" shadows inside the container. |
I'm supporting focusRing
in the static optimizer, not changing it.
#2489 updated style transform to support stencils, but also allowed other style utilities to be transformed outside a `createStyles` or `createStencil` call. Since `createStyles` and `createStencil` have strict requirements for statically analyzable values and other places do not, this can result in odd behavior. For example, calling the following is an incorrect transformation, but present in `Checkbox.tsx`: ```tsx ...focusRing({ width: variant === 'inverse' ? 2 : 0, separation: 0, animate: false, innerColor: variant === 'inverse' ? colors.blackPepper400 : undefined, outerColor: variant === 'inverse' ? colors.frenchVanilla100 : undefined, }), ``` This will result in the following: ```tsx ... { boxShadow: variant === `0 0 0 ${2} ${variant === 'inverse' ? colors.blackPepper400 : undefined}, ...` ``` Since `focusRing` allows `undefined` as an argument with a default fallback, this works at runtime, but the transform doesn't understand the context of runtime. There's no strict check outside static style functions, so the fix is to only run this transform from inside a static style function. [category:Components]
Summary
Fixes: #2463
createStencils
createStyles
,createStencils
, and the transformer.Release Category
Components
Release Note
CSS variables can now be unwrapped anywhere in a property definition when using
createStyles
orcreateStencil
. Previously, unwrapped variables could only be used if they started the property. For example, the following is now valid:You no longer need to use
cssVar
aroundmyVar
to be valid. This should help with the transition from hard-coded variables (const myVar = 'red' => const myVar = '--my-var'
) without needing to wrap in acssVar
. It allows us to think aboutmyVar
as a JS variable and the system will convert to a CSS variable for us.Checklist
ready for review
has been added to PRFor the Reviewer
Where Should the Reviewer Start?
Areas for Feedback? (optional)
Testing Manually
Tests should suffice, but you could always do a
(cd modules/react; yarn clean && yarn build:es6)
and verify thedist
directory. Remember to do ayarn clean
when you're done otherwise your environment is hosed.