-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 support for boxShadow #6749
base: main
Are you sure you want to change the base?
Conversation
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.
Fix CI and we are good to go!
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.
Let's go a bit further. Let's see how boxShadow
fares in both Jest and our runtime tests.
...
…Shadow Runtime test is disabled for now
400f0ce
to
5fa603a
Compare
8f2f601
to
e43a655
Compare
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.
Let's go!! 🥳
|
||
// type BoxShadow = string | BoxShadowValue[]; | ||
|
||
// describe('animation of BoxShadow', () => { |
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.
Instead of commenting out the entire file, you could use the skip
method from the tests
@@ -349,6 +349,10 @@ export const ColorProperties = makeShareable([ | |||
'stroke', | |||
]); | |||
|
|||
const NestedColorProperties = makeShareable({ | |||
boxShadow: ['color'], |
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.
boxShadow: ['color'], | |
boxShadow: 'color', |
I think we can just use a simple string instead of an array since we don't know of any more complicated structures. I understand that using an array would be a more flexible solution, but let's keep it simple for now.
for (const groupKey in nestedPropGroup) { | ||
const nestedProp = nestedPropGroup[groupKey] as StyleProps; | ||
|
||
for (const propName in nestedProp) { | ||
if ( | ||
NestedColorProperties[ | ||
key as keyof typeof NestedColorProperties | ||
].includes(propName) | ||
) { | ||
nestedProp[propName] = processColor(nestedProp[propName]); | ||
} | ||
} |
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.
Instead of iterating over the entire prop object, let's iterate over NestedColorProperties
and check if those properties exist in the prop object because the prop object can potentially have more fields than those listed in NestedColorProperties
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.
prop object being the general object with all the props passed (backgroundColor, width, boxShadow etc.)? Because in current approach I use the loop that was there before (loop through keys in props), and if there is a boxShadow in props (boxShadow being an array by default), I iterate through objects in array (most cases one object), and check for if there is a color prop in boxShadow object
Can you elaborate more? Do you want to move checking the NestedProp outside the parent loop?
* in rgba format, allowing the animation to run correctly. | ||
*/ | ||
if (typeof animation.current === 'object') { | ||
result[key] = { ...animation.current }; |
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.
Do we also need to copy other objects from the style, such as transforms? 🤔
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.
In this approach we also copy transform yes, but it doesn't affect it - it only makes sure any nested the color property will work
if (Array.isArray(updates[propName])) { | ||
updates[propName].forEach((obj: StyleProps) => { | ||
for (const prop in obj) { | ||
last[propName][prop] = obj[prop]; | ||
} | ||
}); | ||
} else { |
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.
- When
updates[propName]
is an array? - What about spreed operator?
- We also need a proper commentary, as this change is not obvious :/
- Is any change to avoid that copying here? - just wondering 🤔
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.
- transform and boxShadow
- spread operator removed problem of reference, but only copying prop to prop assured the independence of
toValue
andcurrent
(in other words it made sure the animation wouldn't jump) - on it!
- in a meaning to avoid copying prop to prop? I tried some other approaches but only this guaranteed smooth animation
Summary
This PR introduces support for boxShadow - a new feature from React Native 0.76+ NewArch. At the same time it address #6687 .
boxShadow
prop, that transforms string into aboxShadow
object.withSpring
Runtime test are added for future fabric support in RuntimeTests.
Fixes #6687
Screen.Recording.2024-11-22.at.16.23.44.mov
Test plan
To test, paste this code snippet to
EmptyExample
and run.EmptyExample code