-
Notifications
You must be signed in to change notification settings - Fork 24.7k
feat: radial gradient #50209
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: radial gradient #50209
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very cool. updated!
var centerY: Float = height / 2f | ||
|
||
if (position.top != null) { | ||
centerY = position.top.resolveToPixel(height) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, interface with getShader
makes most sense, will add it 👍
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.
Updated, will split the PR as Nick mentioned.
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 really exciting! But also a pretty huge change. Could we split this up a bit?
const DIRECTION_KEYWORD_REGEX = | ||
|
||
// Linear Gradient | ||
const LINEAR_GRADIENT_DIRECTION_REGEX = |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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: Adds iOS changes for radial gradient. Previous PR - #50209 ## Changelog: [IOS] [ADDED] - Radial gradient <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #50266 Test Plan: - Added tests in `processBackgroundImage-test.js` in #50268 - Merge this, #50268 and check examples in `RadialGradientExample.js` Reviewed By: joevilches Differential Revision: D71898565 Pulled By: jorge-cab fbshipit-source-id: ac00c7c3cc7dcbe9116c2d60dac8eb1a87153908
Summary: Adds android changes for radial gradient. Previous PR - #50209 ## Changelog: [ANDROID] [ADDED] - Radial gradient <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #50269 Test Plan: - Added tests in processBackgroundImage-test.js in #50268 - Merge this and [this](#50268) and check examples in `RadialGradientExample.js` Reviewed By: NickGerleman Differential Revision: D71898546 Pulled By: jorge-cab fbshipit-source-id: 2872bcf16a492c7bc829be10eedb0f6c7dad32c7
Summary: Adds JS changes for radial gradient. Previous PR - #50209 ## Changelog: [GENERAL] [ADDED] - Radial gradient <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #50268 Test Plan: - Added tests in `processBackgroundImage-test.js` in #50268 - Merge this, #50266 and #50269 and check examples in `RadialGradientExample.js` Reviewed By: NickGerleman Differential Revision: D71898524 Pulled By: jorge-cab fbshipit-source-id: 39bdf0f647ba19839b6e6deb09b30a1c337e6457
Summary: Adds iOS changes for radial gradient. Previous PR - facebook#50209 ## Changelog: [IOS] [ADDED] - Radial gradient <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#50266 Test Plan: - Added tests in `processBackgroundImage-test.js` in facebook#50268 - Merge this, facebook#50268 and check examples in `RadialGradientExample.js` Reviewed By: joevilches Differential Revision: D71898565 Pulled By: jorge-cab fbshipit-source-id: ac00c7c3cc7dcbe9116c2d60dac8eb1a87153908
Summary: Adds android changes for radial gradient. Previous PR - facebook#50209 ## Changelog: [ANDROID] [ADDED] - Radial gradient <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#50269 Test Plan: - Added tests in processBackgroundImage-test.js in facebook#50268 - Merge this and [this](facebook#50268) and check examples in `RadialGradientExample.js` Reviewed By: NickGerleman Differential Revision: D71898546 Pulled By: jorge-cab fbshipit-source-id: 2872bcf16a492c7bc829be10eedb0f6c7dad32c7
Summary: Adds JS changes for radial gradient. Previous PR - facebook#50209 ## Changelog: [GENERAL] [ADDED] - Radial gradient <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#50268 Test Plan: - Added tests in `processBackgroundImage-test.js` in facebook#50268 - Merge this, facebook#50266 and facebook#50269 and check examples in `RadialGradientExample.js` Reviewed By: NickGerleman Differential Revision: D71898524 Pulled By: jorge-cab fbshipit-source-id: 39bdf0f647ba19839b6e6deb09b30a1c337e6457
Summary:
Changelog:
[GENERAL] [ADDED] - Radial gradient
Test Plan:
Added testcases in
processBackgroundImage-test.js
and examples inRadientGradientExample.js