-
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
Merged
roryabraham
merged 49 commits into
Expensify:main
from
rayane-djouah:Migrate-Icon/index.js-to-function-component
Dec 16, 2023
Merged
Changes from 44 commits
Commits
Show all changes
49 commits
Select commit
Hold shift + click to select a range
8b2eca6
Migrate Icon/index.js to function component
rayane-djouah 16a2adf
fix lint errors
rayane-djouah c41e8f2
fix lint errors
rayane-djouah ebf0236
fix errors
rayane-djouah 3ad83ef
declare 'displayName' outside the class body
rayane-djouah ac21c39
run prettier
rayane-djouah 9b50cb8
fix for the fab plus icon, migrate FloatingActionButton to function c…
rayane-djouah 062b7a6
Delete .gitconfig
rayane-djouah 08ac9fe
refactor FloatingActionButton ref use, and renam the file to index.js
rayane-djouah 4f20a47
rename FabPlusIcon component
rayane-djouah 5cb09cc
rename FabPlusIcon component
rayane-djouah c5186bd
Merge branch 'main' into Migrate-Icon/index.js-to-function-component
rayane-djouah 9fe1125
run prettier
rayane-djouah a3c6977
Merge branch 'Expensify:main' into Migrate-Icon/index.js-to-function-…
rayane-djouah 43373cf
remove unused proptypes and add missing ones
rayane-djouah 91482f4
update react-native-svg
rayane-djouah 32b2162
Merge branch 'main' into Migrate-Icon/index.js-to-function-component
rayane-djouah 8e060c3
fix Typescript error
rayane-djouah 8eb99b1
revert latest change
rayane-djouah 96b5d82
Merge branch 'Expensify:main' into Migrate-Icon/index.js-to-function-…
rayane-djouah a8406e0
remove forwarding ref
rayane-djouah a5a253a
Merge branch 'Expensify:main' into Migrate-Icon/index.js-to-function-…
rayane-djouah c055e06
Merge branch 'Expensify:main' into Migrate-Icon/index.js-to-function-…
rayane-djouah 330e50e
Merge branch 'Expensify:main' into Migrate-Icon/index.js-to-function-…
rayane-djouah b5db485
Merge branch 'Expensify:main' into Migrate-Icon/index.js-to-function-…
rayane-djouah b88da06
Fix (On iOS, the + button is showing as a square): Preserve borderRad…
rayane-djouah 1ed2d44
add easing to FabPlusIcon
rayane-djouah 2caca58
Merge 'main' into Migrate-Icon/index.js-to-function-component
rayane-djouah 6647b25
bugfix/react-native-reanimated: Fix animated UI Props on Web #5169
rayane-djouah 31939a4
delete react-native-reanimated patch
rayane-djouah 0795952
fix size
rayane-djouah 6281aa2
move Fab files to components dir
rayane-djouah 90bfde0
Merge branch 'main' into Migrate-Icon/index.js-to-function-component
rayane-djouah 5921a2c
move FAB files to FAB dir
rayane-djouah 5d5ab51
update react-native-reanimated to 3.6.0
rayane-djouah 8c9750f
Merge branch 'main' into Migrate-Icon/index.js-to-function-component
rayane-djouah 67751ef
fix lint errors
rayane-djouah 1fd316c
Merge branch 'Expensify:main' into Migrate-Icon/index.js-to-function-…
rayane-djouah 630c3f6
rename back accessibility role prop
rayane-djouah bc09b8d
update react-native-reanimated to version 3.6.1
rayane-djouah 0b881d4
pod update for react-native-reanimated 3.6.1
rayane-djouah 43adece
Merge branch 'main' into Migrate-Icon/index.js-to-function-component
rayane-djouah d0c7daa
Merge branch 'main' into Migrate-Icon/index.js-to-function-component
rayane-djouah 8964dc9
fix floatingActionButton style use
rayane-djouah 416229d
fix the default props usage and use hooks instead of HOC
rayane-djouah 3648e1b
replace WithTheme and WithThemeStyles HOC with useTheme and useThemeS…
rayane-djouah b42f9f0
Merge branch 'Expensify:main' into Migrate-Icon/index.js-to-function-…
rayane-djouah a6da78c
fix lint error
rayane-djouah 5bc120d
fix failing test
rayane-djouah File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
import PropTypes from 'prop-types'; | ||
import React, {useEffect} from 'react'; | ||
import Animated, {Easing, interpolateColor, useAnimatedProps, useSharedValue, withTiming} from 'react-native-reanimated'; | ||
import Svg, {Path} from 'react-native-svg'; | ||
import themeColors from '@styles/themes/default'; | ||
|
||
const AnimatedPath = Animated.createAnimatedComponent(Path); | ||
|
||
const propTypes = { | ||
/* Current state (active or not active) of the component */ | ||
isActive: PropTypes.bool.isRequired, | ||
}; | ||
|
||
function FabPlusIcon({isActive}) { | ||
const animatedValue = useSharedValue(isActive ? 1 : 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
useEffect(() => { | ||
animatedValue.value = withTiming(isActive ? 1 : 0, { | ||
duration: 340, | ||
easing: Easing.inOut(Easing.ease), | ||
}); | ||
}, [isActive, animatedValue]); | ||
|
||
const animatedProps = useAnimatedProps(() => { | ||
const fill = interpolateColor(animatedValue.value, [0, 1], [themeColors.textLight, themeColors.textDark]); | ||
|
||
return { | ||
fill, | ||
}; | ||
}); | ||
|
||
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} | ||
rayane-djouah marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/> | ||
</Svg> | ||
); | ||
} | ||
|
||
FabPlusIcon.propTypes = propTypes; | ||
FabPlusIcon.displayName = 'FabPlusIcon'; | ||
|
||
export default FabPlusIcon; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
import PropTypes from 'prop-types'; | ||
import React, {useEffect, useRef} from 'react'; | ||
import {View} from 'react-native'; | ||
import Animated, {Easing, interpolateColor, useAnimatedStyle, useSharedValue, withTiming} from 'react-native-reanimated'; | ||
import PressableWithFeedback from '@components/Pressable/PressableWithFeedback'; | ||
import Tooltip from '@components/Tooltip/PopoverAnchorTooltip'; | ||
import withTheme, {withThemePropTypes} from '@components/withTheme'; | ||
import withThemeStyles, {withThemeStylesPropTypes} from '@components/withThemeStyles'; | ||
import useLocalize from '@hooks/useLocalize'; | ||
import compose from '@libs/compose'; | ||
import FabPlusIcon from './FabPlusIcon'; | ||
|
||
const AnimatedPressable = Animated.createAnimatedComponent(PressableWithFeedback); | ||
AnimatedPressable.displayName = 'AnimatedPressable'; | ||
|
||
const propTypes = { | ||
/* Callback to fire on request to toggle the FloatingActionButton */ | ||
onPress: PropTypes.func.isRequired, | ||
rayane-djouah marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/* Current state (active or not active) of the component */ | ||
isActive: PropTypes.bool.isRequired, | ||
|
||
/* An accessibility label for the button */ | ||
accessibilityLabel: PropTypes.string.isRequired, | ||
|
||
/* An accessibility role for the button */ | ||
role: PropTypes.string.isRequired, | ||
|
||
...withThemeStylesPropTypes, | ||
...withThemePropTypes, | ||
}; | ||
|
||
const FloatingActionButton = React.forwardRef(({onPress, isActive, accessibilityLabel, role, theme, themeStyles}, ref) => { | ||
const {translate} = useLocalize(); | ||
const fabPressable = useRef(null); | ||
const animatedValue = useSharedValue(isActive ? 1 : 0); | ||
const buttonRef = ref; | ||
|
||
useEffect(() => { | ||
animatedValue.value = withTiming(isActive ? 1 : 0, { | ||
duration: 340, | ||
easing: Easing.inOut(Easing.ease), | ||
}); | ||
}, [isActive, animatedValue]); | ||
|
||
const animatedStyle = useAnimatedStyle(() => { | ||
const backgroundColor = interpolateColor(animatedValue.value, [0, 1], [theme.success, theme.buttonDefaultBG]); | ||
|
||
return { | ||
transform: [{rotate: `${animatedValue.value * 135}deg`}], | ||
backgroundColor, | ||
borderRadius: themeStyles.floatingActionButton.borderRadius, | ||
}; | ||
}); | ||
|
||
return ( | ||
<Tooltip text={translate('common.new')}> | ||
<View style={themeStyles.floatingActionButtonContainer}> | ||
<AnimatedPressable | ||
ref={(el) => { | ||
fabPressable.current = el; | ||
if (buttonRef) { | ||
buttonRef.current = el; | ||
} | ||
}} | ||
accessibilityLabel={accessibilityLabel} | ||
role={role} | ||
pressDimmingValue={1} | ||
onPress={(e) => { | ||
// Drop focus to avoid blue focus ring. | ||
fabPressable.current.blur(); | ||
onPress(e); | ||
}} | ||
onLongPress={() => {}} | ||
style={[themeStyles.floatingActionButton, animatedStyle]} | ||
> | ||
<FabPlusIcon isActive={isActive} /> | ||
</AnimatedPressable> | ||
</View> | ||
</Tooltip> | ||
); | ||
}); | ||
|
||
FloatingActionButton.propTypes = propTypes; | ||
FloatingActionButton.displayName = 'FloatingActionButton'; | ||
|
||
export default compose(withThemeStyles, withTheme)(FloatingActionButton); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Getting crash on android on latest main
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.
This also might be related
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 inFabPlusIcon.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?
https://github.com/Expensify/App/pull/26654/files#diff-58517876b1c001c5b56c9c446d55dd75c290c4e07c766fdbcb32e0d90d0d42f7R26
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 passadapter
touseAnimatedProps
, see the docs:https://docs.swmansion.com/react-native-reanimated/docs/core/useAnimatedProps/#adapters-
You need the following adapter:
Make sure to import
processColor
fromreact-native-reanimated
(notreact-native
). Then, you pass the adapter as third argument touseAnimatedProps
:Here's the full example for the context:
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:useAnimatedSVGProps
sounds good 🚀