-
Notifications
You must be signed in to change notification settings - Fork 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
Migrate Icon/index.js and FloatingActionButton to function component #26654
Migrate Icon/index.js and FloatingActionButton to function component #26654
Conversation
…omp, add FABPlusIcon custom svg icon
@rushatgabhane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
closed this PR: #25450 and opened this one to trigger the automation on both issues. |
@rushatgabhane PR is ready for review. |
Waiting for merge freeze to be over |
@rushatgabhane, I noticed that other pull requests have been approved by C+ and engineer and are now pending merge. I believe we can proceed with the review and hold the merge after the changes get approved. |
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.
The merge freeze is offically over! @rayane-djouah I request you to merge with the latest main
and re-test at least once, thanks!
@rushatgabhane I merged main and tested again, everything works fine. 29.New.Expensify.Mozilla.Firefox.2023-09-11.11-16-41.mp4 |
ready for review |
@rushatgabhane merge freeze is lifted so feel free to review this when you're ready |
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.
@roryabraham LGTM! all platforms test well
Looks like some PR checks are failing now |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@@ -148,7 +148,7 @@ | |||
"react-native-plaid-link-sdk": "10.8.0", | |||
"react-native-qrcode-svg": "^6.2.0", | |||
"react-native-quick-sqlite": "^8.0.0-beta.2", | |||
"react-native-reanimated": "3.5.4", | |||
"react-native-reanimated": "^3.6.1", |
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.
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.
weird. did you try npm ci? and rebuilding Android
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 removed node_modules, installed npm and did fresh build
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.
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.
@situchan Did you remember to rebuild the native app? Does npm start -- --reset-cache
help?
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.
Already did everything. I had to revert changes on this PR locally to unblock testing other PRs
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.
You're right, I can indeed reproduce this issue on 912fa3f as well. Working on it!
edit: the issue also appears if I downgrade Reanimated from 3.6.1 to 3.5.4. The error comes from fill
property in FabPlusIcon.js
.
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.
@tomekzaw if possible could you please help us understand why the error comes from fill
?
Is the API being used incorrectly?
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.
@rushatgabhane I'm back with answers. Fortunately, it's not a regression in Reanimated 3.6.1 because on 3.5.4 it behaves the same way. It's also not an issue with react-native-svg (13.14.0 is okay).
If you animate fill
prop of SVG elements, you need to pass adapter
to useAnimatedProps
, see the docs:
https://docs.swmansion.com/react-native-reanimated/docs/core/useAnimatedProps/#adapters-
You need the following adapter:
const adapter = createAnimatedPropAdapter(
(props) => {
// delete props.fill;
if (Object.keys(props).includes('fill')) {
props.fill = { type: 0, payload: processColor(props.fill) };
}
},
['fill']
);
Make sure to import processColor
from react-native-reanimated
(not react-native
). Then, you pass the adapter as third argument to useAnimatedProps
:
const animatedProps = useAnimatedProps(
() => {
return {
fill: interpolateColor(animatedValue.value, [0, 1], ['red', 'blue']),
};
},
[],
adapter
);
Here's the full example for the context:
import React, { useEffect } from 'react';
import Animated, {
Easing,
createAnimatedPropAdapter,
interpolateColor,
useAnimatedProps,
useSharedValue,
processColor,
withTiming,
} from 'react-native-reanimated';
import Svg, { Path } from 'react-native-svg';
const AnimatedPath = Animated.createAnimatedComponent(Path);
function FabPlusIcon() {
const animatedValue = useSharedValue(0);
useEffect(() => {
animatedValue.value = withTiming(1, {
duration: 340,
easing: Easing.inOut(Easing.ease),
});
}, [animatedValue]);
const adapter = createAnimatedPropAdapter(
(props) => {
// delete props.fill;
if (Object.keys(props).includes('fill')) {
props.fill = { type: 0, payload: processColor(props.fill) };
}
},
['fill']
);
const animatedProps = useAnimatedProps(
() => {
return {
fill: interpolateColor(animatedValue.value, [0, 1], ['red', 'blue']),
};
},
[],
adapter
);
return (
<Svg width={20} height={20}>
<AnimatedPath
d="M12,3c0-1.1-0.9-2-2-2C8.9,1,8,1.9,8,3v5H3c-1.1,0-2,0.9-2,2c0,1.1,0.9,2,2,2h5v5c0,1.1,0.9,2,2,2c1.1,0,2-0.9,2-2v-5h5c1.1,0,2-0.9,2-2c0-1.1-0.9-2-2-2h-5V3z"
animatedProps={animatedProps}
/>
</Svg>
);
}
FabPlusIcon.displayName = 'FabPlusIcon';
export default FabPlusIcon;
cc @WoLewicki, would be good to provide some abstraction for it, e.g. useAnimatedSVGProps
?
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.
Yeah, I think we should also add stroke
there so it looks something like this:
const adapter = createAnimatedPropAdapter(
(props) => {
if (Object.keys(props).includes('fill')) {
props.fill = { type: 0, payload: processColor(props.fill) };
}
if (Object.keys(props).includes('stroke')) {
props.stroke = {type: 0, payload: processColor(props.stroke)}
}
},
['fill', 'stroke']
);
useAnimatedSVGProps
sounds good 🚀
}, [isActive, animatedValue]); | ||
|
||
const animatedProps = useAnimatedProps(() => { | ||
const fill = interpolateColor(animatedValue.value, [0, 1], [theme.textLight, theme.textDark]); |
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.
FYI if you use foo.bar.baz
inside a worklet, the whole object foo
is captured and copied to the UI runtime. In some cases it's good (maybe even here, since we cache copied objects) but usually it's safer to pass the leaf nodes, e.g.:
// this is regular React code
const { baz } = foo.bar;
useAnimatedProps(() => {
// this is a worklet
// use `baz` directly
});
return { | ||
transform: [{rotate: `${animatedValue.value * 135}deg`}], | ||
backgroundColor, | ||
borderRadius: styles.floatingActionButton.borderRadius, |
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.
Same story for styles
here, be aware that the whole object is copied to the UI runtime (but if it doesn't change then it's cached).
|
||
function FabPlusIcon({isActive}) { | ||
const theme = useTheme(); | ||
const animatedValue = useSharedValue(isActive ? 1 : 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.
const sharedValue
Thank you all, I'm working on a PR now for the fix |
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.14-0 🚀
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.14-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.4.14-6 🚀
|
const styles = useThemeStyles(); | ||
const iconWidth = small ? variables.iconSizeSmall : width; | ||
const iconHeight = small ? variables.iconSizeSmall : height; | ||
const iconStyles = [StyleUtils.getWidthAndHeightStyle(width ?? 0, height), IconWrapperStyles, styles.pAbsolute, additionalStyles]; |
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.
Why don't we use iconWidth
and iconHeight
here? I think they are the correct one compared to the original file.
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.
Yes, you're right @tienifr, we missed this.
testID={`${this.props.src.name} Icon`} | ||
style={this.props.additionalStyles} | ||
testID={`${src.name} Icon`} | ||
style={[StyleUtils.getWidthAndHeightStyle(width ?? 0, height), styles.bgTransparent, styles.overflowVisible]} |
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.
Same as above.
Details
Migrate Icon/index.js to function component.
Migrate FloatingActionButton.js to function component.
Moved FloatingActionButton.js and introduced new file FABPlusIcon.js within src/components/FloatingActionButton/. These break the logic and presentation of the previous FloatingActionButton into a modular approach.
Fixed Issues
$ #16163
$ #16150
Tests
FAB Animation test:
Offline tests
N/A
QA Steps
same as tests above.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
web.mp4
Mobile Web - Chrome
chrome.mp4
Mobile Web - Safari
safari.mp4
Desktop
desktop.mp4
iOS
ios.mp4
Android
android.mp4