[RNMobile] Refactor native gradient logic #27307
Conversation
lukewalczak
added
the
Mobile App - i.e. Android or iOS
|
Size Change: +18 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
lukewalczak
requested review from
ajitbohra and
chrisvanpatten
as code owners
| @@ -21,7 +21,7 @@ export function serializeGradientColorStop( { type, value, length } ) { | |||
| return `${ serializeGradientColor( { | |||
| type, | |||
| value, | |||
| } ) } ${ serializeGradientPosition( length ) }`; | |||
| } ) } ${ length ? serializeGradientPosition( length ) : '' }`; | |||
There was a problem hiding this comment.
Gradient color length (gradient color location - e.g. 50%) can be undefined e.g. in linear-gradient(to top , #3C8067, #FAFBF6) from the theme called Spearhead. On web nothing happens when trying to switch from linear to radial but on mobile there is a crash since we reuse logic within serializer.js.
Properties of undefined length are then used in function:
export function serializeGradientPosition( { type, value } ) {
return `${ value }${ type }`;
}
which makes a crash.
There was a problem hiding this comment.
To me this seems ok to do, but this code is new to me as well. Maybe @jorgefilipecosta can have a quick look?
There was a problem hiding this comment.
This change looks good to me 👍
I guess as an enhancement we can add this logic inside the serializeGradientPosition function directly:
export function serializeGradientPosition( position ) {
if ( ! position ) {
return '';
}
const { value, type } = position;
return `${ value }${ type }`;
}
|
Hi @ajitbohra @chrisvanpatten 👋 , a review on this update to The change is useful for fixing a crash in native mobile apps. I can review the rest of the PR from the mobile perspective. 🙇 |
|
Tested successfully from a mobile perspective 👍 . I see the change fixed the crash when setting the custom gradient on the cover block for the Spearhead Theme. This change is ready to go as long as the web side change checks out with necessary code owners. |
|
|
|
The failing job also fails on master so, doesn't seem like this PR is introducing the fail. |
| @@ -21,7 +21,7 @@ export function serializeGradientColorStop( { type, value, length } ) { | |||
| return `${ serializeGradientColor( { | |||
| type, | |||
| value, | |||
| } ) } ${ serializeGradientPosition( length ) }`; | |||
| } ) } ${ length ? serializeGradientPosition( length ) : '' }`; | |||
There was a problem hiding this comment.
This change looks good to me 👍
I guess as an enhancement we can add this logic inside the serializeGradientPosition function directly:
export function serializeGradientPosition( position ) {
if ( ! position ) {
return '';
}
const { value, type } = position;
return `${ value }${ type }`;
}
|
@Tug Is there an option to paste a HTML of that post? |
I will tackle it as a next task I think, that logic definitely need some unit tests. |
+1 for tackling that or similar tasks after we merge and include this fix to the ongoing mobile editor release that's due today. |
|
My bad it's working fine, I had trouble with my environment. |
|
Looks like #25854 broke CI tests on |
* Refactor native gradient logic * Make using 'length' conditional in serializing * Move fixing logic directly to serializeGradientPosition * Remove global from regex
* Release script: Update react-native-editor version to 1.42.0 * Release script: Update with changes from 'npm run core preios' * [RNMobile] Make initial value passed to slider always a number (#27273) * [RNMobile] Refactor native gradient logic (#27307) * Refactor native gradient logic * Make using 'length' conditional in serializing * Move fixing logic directly to serializeGradientPosition * Remove global from regex Co-authored-by: Luke Walczak <[email protected]>
* Release script: Update react-native-editor version to 1.42.0 * Release script: Update with changes from 'npm run core preios' * [RNMobile] Make initial value passed to slider always a number (#27273) * [RNMobile] Refactor native gradient logic (#27307) * Refactor native gradient logic * Make using 'length' conditional in serializing * Move fixing logic directly to serializeGradientPosition * Remove global from regex * Release script: Update react-native-editor version to 1.42.1 * Release script: Update with changes from 'npm run core preios' Co-authored-by: Cameron Voell <[email protected]> Co-authored-by: Luke Walczak <[email protected]>
Description
Refactor the parsing gradient which is passed to rn linear gradient component.
How has this been tested?
Set theme
Twenty TwentyOpen a post
Add cover block
Choose the background color
Press cog to open settings
Go to color and press
GradientExpect there is no crash
Choose one of the gradients
Save the post
Exit the editor and switch theme to
SpearheadKill the app and reopen it (known issue with themes)
Go to the previous post
Expect cover background is not set
Choose the background color
Press cog to open settings
Go to color and press
GradientExpect there is no crash and expect to see the following gradients:
TO INVESTIGATE
Once trying to convert marked gradients to radial gradient (press
customize gradientthenradial) app crashes. I've checked that on the web and there user cannot convert these two gradients to linear.I push the commit with the fix and left the comment
Screenshots
Types of changes
Checklist: