Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
dad3f55
smooth animation
sumo-slonik Dec 11, 2024
457b549
work in progress
sumo-slonik Dec 11, 2024
eaa7d47
clean code before PR
sumo-slonik Dec 11, 2024
d852938
animation works in all places
sumo-slonik Dec 12, 2024
2b2b451
refactor before PR
sumo-slonik Dec 12, 2024
c2fb131
changes after code review
sumo-slonik Dec 13, 2024
4473400
changes after Blazej review
sumo-slonik Dec 16, 2024
7443bc5
fix padding problem in toggle page
sumo-slonik Dec 17, 2024
6c8771d
Merge branch 'main' into feature/kuba_nowakowski/add_animation_for_sw…
sumo-slonik Dec 17, 2024
24cd3ca
fix eslint
sumo-slonik Dec 17, 2024
8b7525a
Merge branch 'main' into feature/kuba_nowakowski/add_animation_for_sw…
sumo-slonik Dec 18, 2024
23c3b57
work in progress
sumo-slonik Dec 19, 2024
408750f
adding accordion to all places, remove unnecessary callback in switch…
sumo-slonik Dec 20, 2024
be4015e
add easing to effect
sumo-slonik Dec 20, 2024
53fd209
fix liner reanimated API
sumo-slonik Jan 7, 2025
314dca1
remove unnecessary const
sumo-slonik Jan 7, 2025
d99e589
fix linter
sumo-slonik Jan 7, 2025
20401d1
in progress
sumo-slonik Jan 10, 2025
007eac8
finish adding accordion without refactor
sumo-slonik Jan 10, 2025
65f680d
first part of refactor
sumo-slonik Jan 10, 2025
09794bf
Fix animation on first appearance
sumo-slonik Jan 13, 2025
b9aed8e
fix animation using navigation
sumo-slonik Jan 13, 2025
a23be73
Revert changes different from the workflows page.
sumo-slonik Jan 13, 2025
67a6187
Merge branch 'main' into feature/kuba_nowakowski/add_animation_for_sw…
sumo-slonik Jan 14, 2025
a65661f
change suggested by Vit
sumo-slonik Jan 14, 2025
d7e1334
works almost perfect
sumo-slonik Jan 14, 2025
a174547
works perfect
sumo-slonik Jan 14, 2025
8c39088
ready for tests
sumo-slonik Jan 15, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6437,6 +6437,8 @@ const CONST = {
},

MIGRATED_USER_WELCOME_MODAL: 'migratedUserWelcomeModal',

DEFAULT_POLICY_ID: '-1',
Comment thread
sumo-slonik marked this conversation as resolved.
Outdated
} as const;

type Country = keyof typeof CONST.ALL_COUNTRIES;
Expand Down
45 changes: 45 additions & 0 deletions src/components/Accordion/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import type {ReactNode} from 'react';
import React from 'react';
import {View} from 'react-native';
import type {SharedValue} from 'react-native-reanimated';
import Animated, {useAnimatedStyle, useDerivedValue, useSharedValue, withTiming} from 'react-native-reanimated';
import useThemeStyles from '@hooks/useThemeStyles';

type AccordionProps = {
/** Giving information whether the component is open */
isExpanded: SharedValue<boolean>;

/** Element that is inside Accordion */
children: ReactNode;

/** Duration of expansion animation */
duration?: number;
};

function Accordion({isExpanded, children, duration = 300}: AccordionProps) {
const height = useSharedValue(0);
const styles = useThemeStyles();
const derivedHeight = useDerivedValue(() =>
withTiming(height.get() * Number(isExpanded.get()), {
duration,
}),
);
const bodyStyle = useAnimatedStyle(() => ({
height: derivedHeight.get(),
}));

return (
<Animated.View style={[bodyStyle, styles.overflowHidden]}>
<View
onLayout={(e) => {
height.set(e.nativeEvent.layout.height);
}}
style={[styles.pAbsolute, styles.l0, styles.r0, styles.t0]}
>
{children}
</View>
</Animated.View>
);
}
Comment thread
sumo-slonik marked this conversation as resolved.

export default Accordion;
13 changes: 11 additions & 2 deletions src/components/Switch.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,30 @@ type SwitchProps = {

/** Callback to fire when the switch is toggled in disabled state */
disabledAction?: () => void;

/**
* Callback function triggered only after a successful change
* in the external state of the switch, after the switch state change animation is triggered
*/
onStateChange?: (isOn: boolean) => void;
};

const OFFSET_X = {
OFF: 0,
ON: 20,
};

function Switch({isOn, onToggle, accessibilityLabel, disabled, showLockIcon, disabledAction}: SwitchProps) {
function Switch({isOn, onToggle, accessibilityLabel, disabled, showLockIcon, disabledAction, onStateChange}: SwitchProps) {
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);
Copy link
Copy Markdown
Contributor

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? 🤔

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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.

}
}, [onStateChange, isOn, offsetX]);

const handleSwitchPress = () => {
InteractionManager.runAfterInteractions(() => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import {Str} from 'expensify-common';
import React from 'react';
import React, {useEffect, useState} from 'react';
import {useSharedValue} from 'react-native-reanimated';
import Accordion from '@components/Accordion';
import ConnectionLayout from '@components/ConnectionLayout';
import MenuItemWithTopDescription from '@components/MenuItemWithTopDescription';
import OfflineWithFeedback from '@components/OfflineWithFeedback';
Expand Down Expand Up @@ -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;
Copy link
Copy Markdown
Contributor

@blazejkustra blazejkustra Dec 19, 2024

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

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const config = policy?.connections?.intacct?.config;
const translationKeys = getDisplayTypeTranslationKeys(config?.mappings?.[mappingName]);
const isImportMappingEnable = config?.mappings?.[mappingName] !== CONST.SAGE_INTACCT_MAPPING_VALUE.NONE;
const isAccordionExpanded = useSharedValue(isImportMappingEnable);

// 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.
const [translationKeys, setTranslationKey] = useState<DisplayTypeTranslationKeys | undefined>(undefined);

useEffect(() => {
if (!isImportMappingEnable) {
return;
}
setTranslationKey(getDisplayTypeTranslationKeys(config?.mappings?.[mappingName]));
Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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

}, [isImportMappingEnable, config?.mappings, mappingName]);

return (
<ConnectionLayout
displayName={SageIntacctToggleMappingsPage.displayName}
Expand All @@ -81,12 +95,13 @@ function SageIntacctToggleMappingsPage({route}: SageIntacctToggleMappingsPagePro
onToggle={(enabled) => {
const mappingValue = enabled ? CONST.SAGE_INTACCT_MAPPING_VALUE.TAG : CONST.SAGE_INTACCT_MAPPING_VALUE.NONE;
updateSageIntacctMappingValue(policyID, mappingName, mappingValue, config?.mappings?.[mappingName]);
isAccordionExpanded.set(enabled);
}}
pendingAction={settingsPendingAction([mappingName], config?.pendingFields)}
errors={ErrorUtils.getLatestErrorField(config ?? {}, mappingName)}
onCloseError={() => clearSageIntacctErrorField(policyID, mappingName)}
/>
{isImportMappingEnable && (
<Accordion isExpanded={isAccordionExpanded}>
<OfflineWithFeedback pendingAction={settingsPendingAction([mappingName], config?.pendingFields)}>
<MenuItemWithTopDescription
title={translationKeys?.titleKey ? translate(translationKeys?.titleKey) : undefined}
Expand All @@ -102,7 +117,7 @@ function SageIntacctToggleMappingsPage({route}: SageIntacctToggleMappingsPagePro
{translationKeys?.descriptionKey ? translate(translationKeys?.descriptionKey) : undefined}
</Text>
</OfflineWithFeedback>
)}
</Accordion>
</ConnectionLayout>
);
}
Expand Down
9 changes: 8 additions & 1 deletion src/pages/workspace/workflows/ToggleSettingsOptionRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import type {ReactNode} from 'react';
import React, {useMemo} from 'react';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import React, {useMemo} from 'react';
import React, {useMemo, useEffect} from 'react';

import {View} from 'react-native';
import type {StyleProp, TextStyle, ViewStyle} from 'react-native';
import {useSharedValue} from 'react-native-reanimated';
import Accordion from '@components/Accordion';
import Icon from '@components/Icon';
import OfflineWithFeedback from '@components/OfflineWithFeedback';
import RenderHTML from '@components/RenderHTML';
Expand Down Expand Up @@ -98,6 +100,10 @@ function ToggleSettingOptionRow({
showLockIcon = false,
}: ToggleSettingOptionRowProps) {
const styles = useThemeStyles();
const isExpanded = useSharedValue(isActive);
const setIsExpanded = (value: boolean) => {
isExpanded.set(value);
};

const subtitleHtml = useMemo(() => {
if (!subtitle || !shouldParseSubtitle || typeof subtitle !== 'string') {
Expand Down Expand Up @@ -175,10 +181,11 @@ function ToggleSettingOptionRow({
isOn={isActive}
disabled={disabled}
showLockIcon={showLockIcon}
onStateChange={setIsExpanded}
/>
</View>
{shouldPlaceSubtitleBelowSwitch && subtitle && subTitleView}
{isActive && subMenuItems}
<Accordion isExpanded={isExpanded}>{subMenuItems}</Accordion>
</View>
</OfflineWithFeedback>
);
Expand Down