feat: radial gradient #50209
feat: radial gradient #50209
Conversation
facebook-github-bot
added
CLA Signed
There was a problem hiding this comment.
Only a few things on initial review! Thank you for working on this!
I will import to test the "meat" of the algorithm more thoroughly and for it to go through internal review. I will try to make it so it doesn't take as much time to land this as the other times.
| #include <react/renderer/graphics/ColorStop.h> | ||
| #import <vector> | ||
|
|
||
| using namespace facebook::react; |
There was a problem hiding this comment.
We shouldn't use using namespace on header files since anything that might include the header will also get the using namespace so lets remove it from here
| public fun resolveToPixel(referenceLength: Float): Float { | ||
| if (type == LengthPercentageType.PERCENT) { | ||
| return (value / 100) * referenceLength | ||
| } | ||
| return PixelUtil.toPixelFromDIP(value) | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think this is needed, we have .dpToPx() method added to Floats so I think we should just do that when using the resolved value. Specially since this would be adding yet another public API
There was a problem hiding this comment.
very cool. updated!
| var centerY: Float = height / 2f | ||
|
|
||
| if (position.top != null) { | ||
| centerY = position.top.resolveToPixel(height) |
There was a problem hiding this comment.
For this and all other usages of resolveToPixel
| centerY = position.top.resolveToPixel(height) | |
| centerY = position.top.resolve(height).dpToPx() |
|
@jorge-cab has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| internal class Gradient(gradient: ReadableMap?, context: Context) { | ||
| private enum class GradientType { | ||
| LINEAR_GRADIENT | ||
| LINEAR_GRADIENT, | ||
| RADIAL_GRADIENT | ||
| } | ||
|
|
||
| private val type: GradientType | ||
| private val linearGradient: LinearGradient | ||
| private val linearGradient: LinearGradient? | ||
| private val radialGradient: RadialGradient? |
There was a problem hiding this comment.
It feels like there should be a better way to do this. Maybe have Gradient be a super class for LinearGradient and RadialGradient? Or maybe have it be an interface where we implement getShader and do the parsing of LinearGradient/RadialGradient on their own class?
There was a problem hiding this comment.
true, interface with getShader makes most sense, will add it 👍
There was a problem hiding this comment.
Updated, will split the PR as Nick mentioned.
| const DIRECTION_KEYWORD_REGEX = | ||
|
|
||
| // Linear Gradient | ||
| const LINEAR_GRADIENT_DIRECTION_REGEX = |
There was a problem hiding this comment.
Heads up, that we are starting soon on iOS to experiment with the native parser which replaced ViewConfig processors. I didn't port this for backgroundImage yet, but the other processors should all be there with tests. https://github.com/facebook/react-native/blob/main/packages/react-native/ReactCommon/react/renderer/css/CSSTransformOrigin.h
This is building in OSS now, but IDK the story for GTest. @cortinico would know better.
There was a problem hiding this comment.
@NickGerleman yes! i saw this one. i didn't spend much time to try running it as currently the flag was off. But i will try to write a native parser for gradients 😄.
For now, i can split this PR into three (iOS, android and JS changes)
|
@NickGerleman @jorge-cab i have split the PRs into three parts. Feel free to close this one.
To test on a platform, merge JS and that platform's PR. Sorry for the PR spam 😅 |
Summary:
Changelog:
[GENERAL] [ADDED] - Radial gradient
Test Plan:
Added testcases in
processBackgroundImage-test.jsand examples inRadientGradientExample.js