-
Notifications
You must be signed in to change notification settings - Fork 24.7k
feat: radial gradient js prop passing #50268
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 js prop passing #50268
Conversation
@jorge-cab has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Just found a few bugs. We crash on 0 position on Android Haven't looked into iOS yet but might find some more stuff there. Sorry I'm doing multiple reviewing rounds like this but it helps me keep my sanity with such a huge change. Again thank you for working on this!! |
type RadialExtent = | ||
| 'closest-corner' | ||
| 'closest-side' | ||
| 'farthest-corner' | ||
| 'farthest-side'; |
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 a re-definition? Can we import this and RadialGradientPosition from stylesheettypes?
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.
My bad. Fixed here
let hasExplicitShape = false; | ||
const firstPartTokens = firstPartStr.split(/\s+/); | ||
|
||
while (firstPartTokens.length > 0) { |
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.
So all the parsing done with firstPartTokens
is the position part of radial-gradient? Can we maybe move to a function parseRadialGradientPosition
so that it is easier to reason about?
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.
firstPartTokens
could be shape, size and positions e.g. radial-gradient(circle 100px at top left, red, green)
. Here, circle 100px at top left
is the firstPartTokens
. If none of these then they could be color stops. e.g. radial-gradient(red, green)
. I added a comment, maybe that could do the job? But let me know!
@@ -891,4 +891,242 @@ describe('processBackgroundImage', () => { | |||
{color: processColor('blue'), position: 40}, | |||
]); | |||
}); | |||
|
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.
Mentioned it on the Android PR but lets add test for 0 radius radial-gradient(0px 0px at center, red, blue) and negative radius radial-gradient(-10px -10px at center, red, blue) both here and on the RNTester example to make sure its rendering the same as web
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.
const gradients = []; | ||
let match; | ||
// matches one or more linear-gradient or radial-gradient functions in CSS | ||
const gradientRegex = | ||
/(linear|radial)-gradient\s*\(((?:\([^)]*\)|[^()])*)\)/gi; |
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.
Just noticing this now but this approach allows stuff like aoeusradial-gradient(...)
and aoeulinear-gradient(...)
so regex might need some edits since this should fail?
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.
Fixed here!
right: number | string, | ||
}; | ||
type RadialGradientValue = { | ||
type: '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.
I guess this is existing, but it seems odd that this type is camelCase
compared to others which are kebab-case
Can we change for consistency?
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.
Fixed and pushed on all the PRs
@@ -11,23 +11,24 @@ | |||
'use strict'; | |||
|
|||
import type {ProcessedColorValue} from './processColor'; | |||
import type {GradientValue} from './StyleSheetTypes'; | |||
|
|||
import type {BackgroundImageValue} from './StyleSheetTypes'; |
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 file is starting to do a lot of things 😅. Any way we could split the structure to more distinct files/functions for e.g. linear gradient vs radial gradient vs url vs layer list?
Note that on Android, we added some checks for UIManagerType to only allow some new features when using Fabric. This is to avoid confusing parity gaps between Android and iOS (where iOS Paper has separate impl we are not touching), but it's also to let us delete all the ViewConfig processors for new features, once the native parsing stack (which will be Fabric only) is rolled out.
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.
Any way we could split the structure
yes, how about we add a directory StyleSheet/BackgroundImage
and create files like linearGradient
, radialGradient
, types
and utils
inside of it. I wanted to do that but kept it like this to keep it consistent with other files. 😅
@jorge-cab has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
2 similar comments
@jorge-cab has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@jorge-cab has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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
@jorge-cab merged this pull request in 1b45dc8. |
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:
Adds JS changes for radial gradient. Previous PR - #50209
Changelog:
[GENERAL] [ADDED] - Radial gradient
Test Plan:
processBackgroundImage-test.js
in feat: radial gradient js prop passing #50268RadialGradientExample.js