-
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
Adding animation for children of switch components #53938
base: main
Are you sure you want to change the base?
Adding animation for children of switch components #53938
Conversation
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
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.
Looks mostly ok. There is one thing with the translation keys and state that is a bit confusing. If you cannot rewrite it in any way, then at least drop a comment.
Otherwise lgtm 👍
You have a typo in name of the PR: childreans
-> childrens
src/components/Acordition/index.tsx
Outdated
import type {SharedValue} from 'react-native-reanimated'; | ||
import Animated, {useAnimatedStyle, useDerivedValue, useSharedValue, withTiming} from 'react-native-reanimated'; | ||
|
||
function AccordionItem({isExpanded, children, duration = 300}: {isExpanded: SharedValue<boolean>; children: ReactNode; duration?: number}) { |
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.
Please make sure the file and main export are named the same:
Component is called AccordionItem
but the file is Accordion/index.ts
(you have a type in the name).
src/components/Acordition/index.tsx
Outdated
})); | ||
|
||
return ( | ||
<Animated.View style={[bodyStyle, {overflow: 'hidden'}]}> |
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.
<Animated.View style={[bodyStyle, {overflow: 'hidden'}]}> | |
<Animated.View style={[bodyStyle, styles.overflowHidden]}> |
where styles come from:
const styles = useThemeStyles();
src/components/Switch.tsx
Outdated
@@ -26,21 +26,30 @@ type SwitchProps = { | |||
|
|||
/** Callback to fire when the switch is toggled in disabled state */ | |||
disabledAction?: () => void; | |||
|
|||
/** | |||
* onStateChange: Callback function triggered only after the external state of the switch has successfully changed. |
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.
* onStateChange: Callback function triggered only after the external state of the switch has successfully changed. | |
* Callback function triggered only after the external state of the switch has successfully changed. |
repeating the name is not needed, as the name of prop is right below this comment
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 feel like this comment is a bit inaccurate.
This callback is triggered inside Switch
component after the external state has changed AND the animation has finished, right?
Because if it was only after state change, then the external component could handle this itself. So I'm guessing it is important that this runs after the line:
offsetX.set(withTiming([...])...
Does this make sense?
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 after the animation of moving the circle inside the switch, but it is this callback that actually triggers in our case the animation of rolling up and unrolling children
if (!isImportMappingEnable) { | ||
return; | ||
} | ||
setTranslationKey(getDisplayTypeTranslationKeys(config?.mappings?.[mappingName])); |
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 line will be confusing, since assigning translation keys to state is quite uncommon.
Perhaps we could add a short 1-line comment to at least say its done for animation purpose?
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 problem here is that changing the switch changes the content of the object:
config.mappings
which resulted in changing the content of the translation itself, that's why I changed the assignment here to only non-empty values, I was afraid of operating on the config logic itself so I think it's safe to leave a comment here
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
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.
LGTM 👍 the animation is tricky but I was not able to come up with a better solution
@dubielzyk-expensify @thesahindia One of you needs to 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] |
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.
Great work @sumo-slonik 🚀
// We are storing translation keys in the local state for animation purposes. | ||
// Otherwise, the values change to undefined immediately after clicking, before the closing animation finishes, | ||
// resulting in a janky animation effect. | ||
|
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.
src/components/Switch.tsx
Outdated
const styles = useThemeStyles(); | ||
const offsetX = useSharedValue(isOn ? OFFSET_X.ON : OFFSET_X.OFF); | ||
const theme = useTheme(); | ||
|
||
useEffect(() => { | ||
offsetX.set(withTiming(isOn ? OFFSET_X.ON : OFFSET_X.OFF, {duration: 300})); | ||
}, [isOn, offsetX]); | ||
if (onStateChange) { | ||
onStateChange(isOn); |
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.
Can't we use onToggle
for this? 🤔
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.
Unfortunately it seems to me that it does not, because doing it in onToggle called the animation trigger at the wrong time, but I will make sure that in the final version of the code it did not become possible
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 was able to remove the use of this method and it is no longer needed.
src/components/Accordion/index.tsx
Outdated
import Animated, {useAnimatedStyle, useDerivedValue, useSharedValue, withTiming} from 'react-native-reanimated'; | ||
import useThemeStyles from '@hooks/useThemeStyles'; | ||
|
||
function Accordion({isExpanded, children, duration = 300}: {isExpanded: SharedValue<boolean>; children: ReactNode; duration?: number}) { |
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.
Let's extract props type to a separate type above, a good rule of thumb is to always do it for complex types like this
src/components/Accordion/index.tsx
Outdated
onLayout={(e) => { | ||
height.set(e.nativeEvent.layout.height); | ||
}} | ||
style={{position: 'absolute', left: 0, right: 0, top: 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.
Let's use styling utilities here:
styles.pAbsolute, styles.t0 ...
@dubielzyk-expensify good catch, I'll fix it |
@dubielzyk-expensify Screen.Recording.2024-12-17.at.10.53.55.mov |
Agree that the hint text should be included in the row hover. In terms of your two animations Jon, I like both of them - I am guessing the version that only includes a fadeIn is probably easier to build? |
From my point of view, it will take me about the same amount of time to build the first and second animation, so I will do the one that you like more. Regarding the help text when you hover the cursor, I didn't change anything here and it is exactly as it was before. Changing this is not hard from a technical point of view, but it will affect the whole application and all the places that use MenuItem so it would probably require additional QA work. |
Feel free to dk the one which will be simpler to implement |
What's different in the markup between the one in your video vs the one you find on Settings -> Workspace -> Profile then?
Awesome. Let's start with the first one and we can tweak it later if we want. |
I was able to fix the hover effect, because of your example with the default currency. Screen.Recording.2024-12-19.at.17.02.24.movI added opacity animation however I made it as an animation between 0 and 1, on your video it looked like some opacity gradient, doing something like this will take much more time |
I think that's looking pretty good. @Expensify/design for 👀 as well. |
This feels pretty good too.
Perhaps Jon was just using an easing function. Are we able to do something like |
@@ -50,11 +52,23 @@ function SageIntacctToggleMappingsPage({route}: SageIntacctToggleMappingsPagePro | |||
|
|||
const policy = usePolicy(route.params.policyID); | |||
const mappingName: SageIntacctMappingName = route.params.mapping; | |||
const policyID: string = policy?.id ?? '-1'; | |||
|
|||
const policyID: string = policy?.id ?? CONST.DEFAULT_POLICY_ID; |
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.
Please read through contributingGuides/STYLE.md
(Default value for inexistent IDs). I think we shouldn't add DEFAULT_POLICY_ID
and instead use no default id at all, what do you think? @sumo-slonik
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, let's avoid any defaulting values for string ids!
In this case maybe you can try to use route.params.policyID
- this way you won't need to handle undefined cases, wdyt?
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 seems to me that the example in contributingGuides/STYLE.md
indicates that we should have no value, but I wanted to keep the sense of the code. But I will do it as you suggest.
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 please lets remove this
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.
@thesahindia how is it looking? |
Yeah an |
Are we only updating Sage intacct page in this PR? Screen.Recording.2024-12-20.at.9.56.26.AM.mov |
When you toggle the switch for an item at the bottom of the page, you have to scroll yourself to see the content at the bottom (Unrelated to this PR) 1.movIf the switch is in the enabled state then it works fine 2.movShould we also work on the animation for the items in the settings list or will it be tackled separately? Screen.Recording.2024-12-20.at.9.54.14.AM.mov |
I'll fix it.
I'm fixing it so it will work for the others too |
It seems to me that this should not be a part of this PR, but if you think it would be good to take care of it I will be happy to take care of it in the new one (not sure if there should be a new issue) |
…, fix hover effect
I tend to agree that we should not mess with the left hand nav right now. Let's keep this PR focused. @Expensify/design can weigh in too, but I feel like it would be better to tackle those in a separate issue. |
Yeah, makes sense to me 👍 |
@thesahindia how is it going on testing part? |
@sumo-slonik @thesahindia Would like to confirm, what is the nest step for this pr. Can you please fix the Prettier check? Thanks! |
I have to finish the final testing and complete the checklist. I will do it soon. |
I merged the newest main and fixed prettier for you @thesahindia. @mountiny I think it is ready for testing and C+ review :) |
@@ -1,7 +1,9 @@ | |||
import type {ReactNode} from 'react'; | |||
import {ReactNode, useEffect} from '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.
import {ReactNode, useEffect} from 'react'; | |
import type {ReactNode} from 'react'; |
@@ -1,7 +1,9 @@ | |||
import type {ReactNode} from 'react'; | |||
import {ReactNode, useEffect} from 'react'; | |||
import React, {useMemo} from '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.
import React, {useMemo} from 'react'; | |
import React, {useMemo, useEffect} from 'react'; |
@@ -6440,6 +6440,8 @@ const CONST = { | |||
}, | |||
|
|||
MIGRATED_USER_WELCOME_MODAL: 'migratedUserWelcomeModal', | |||
|
|||
DEFAULT_POLICY_ID: '-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.
DEFAULT_POLICY_ID: '-1', |
@@ -50,11 +52,23 @@ function SageIntacctToggleMappingsPage({route}: SageIntacctToggleMappingsPagePro | |||
|
|||
const policy = usePolicy(route.params.policyID); | |||
const mappingName: SageIntacctMappingName = route.params.mapping; | |||
const policyID: string = policy?.id ?? '-1'; | |||
|
|||
const policyID: string = policy?.id ?? CONST.DEFAULT_POLICY_ID; |
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.
@@ -1,5 +1,7 @@ | |||
import React, {useCallback, useEffect} from 'react'; | |||
import {View} from 'react-native'; |
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.
Explanation of Change
These PR add animations of the appearance and disappearance of elements after the switch using reanimated.
Fixed Issues
$ #53759
PROPOSAL:
Tests
Offline tests
Offline tests are not needed.
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
android.native.mp4
Android: mWeb Chrome
web.android.mp4
iOS: Native
ios.native.mp4
iOS: mWeb Safari
web.ios.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4