Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: re-add finetunings #42026

Merged
merged 4 commits into from
May 27, 2024
Merged

fix: re-add finetunings #42026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
41c105d
fix: re-add finetunings
allroundexperts May 12, 2024
b72a7ac
fix: add timeout
allroundexperts May 12, 2024
ad79bcc
fix: add timeout
allroundexperts May 12, 2024
74e8fc2
revert podfile changes
allroundexperts May 12, 2024
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
10 changes: 8 additions & 2 deletions src/CONST.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,13 @@ const CONST = {
// Note: Group and Self-DM excluded as these are not tied to a Workspace
WORKSPACE_ROOM_TYPES: [chatTypes.POLICY_ADMINS, chatTypes.POLICY_ANNOUNCE, chatTypes.DOMAIN_ALL, chatTypes.POLICY_ROOM, chatTypes.POLICY_EXPENSE_CHAT],
ANDROID_PACKAGE_NAME,
ANIMATED_HIGHLIGHT_DELAY: 500,
ANIMATED_HIGHLIGHT_DURATION: 500,
WORKSPACE_ENABLE_FEATURE_REDIRECT_DELAY: 100,
ANIMATED_HIGHLIGHT_ENTRY_DELAY: 50,
ANIMATED_HIGHLIGHT_ENTRY_DURATION: 300,
ANIMATED_HIGHLIGHT_START_DELAY: 10,
ANIMATED_HIGHLIGHT_START_DURATION: 300,
ANIMATED_HIGHLIGHT_END_DELAY: 800,
ANIMATED_HIGHLIGHT_END_DURATION: 2000,
ANIMATED_TRANSITION: 300,
ANIMATED_TRANSITION_FROM_VALUE: 100,
ANIMATION_IN_TIMING: 100,
Expand All @@ -91,6 +96,7 @@ const CONST = {
// Multiplier for gyroscope animation in order to make it a bit more subtle
ANIMATION_GYROSCOPE_VALUE: 0.4,
BACKGROUND_IMAGE_TRANSITION_DURATION: 1000,
SCREEN_TRANSITION_END_TIMEOUT: 1000,
ARROW_HIDE_DELAY: 3000,

API_ATTACHMENT_VALIDATIONS: {
Expand Down
9 changes: 8 additions & 1 deletion src/components/ScreenWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,18 @@ function ScreenWrapper(
).current;

useEffect(() => {
// On iOS, the transitionEnd event doesn't trigger some times. As such, we need to set a timeout
const timeout = setTimeout(() => {
setDidScreenTransitionEnd(true);
onEntryTransitionEnd?.();
}, CONST.SCREEN_TRANSITION_END_TIMEOUT);

Copy link
Contributor

@sobitneupane sobitneupane May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@allroundexperts I'm wondering if there's a way to avoid using setTimeout. In which cases transitionEnd event doesn't trigger? Is this a known issue? Perhaps we could troubleshoot and resolve it first so that we don't have to rely on the setTimeout workaround.

cc: @mountiny

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned here, this is a bug in react-native-navigation. For iOS, it doesn't seem to call the transition end handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also wonder if there is any good way to avoid using setTimeout 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think using setTimeout here isn't a bad practice. It's more of a safe exit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my worry is that we're changing the value of didScreenTransitionEnd using setTimeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, so it is a known bug on iOS that sometimes didScreenTransitionEnd never gets set to true even though the transition is actually finished. Setting a setTimeout ensures that we handle such cases. Does that make sense @hayata-suenaga? Do you have any alternative that I should explore?

Copy link
Contributor

@hayata-suenaga hayata-suenaga May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with you, @allroundexperts 😄. However, as we aim to avoid setTimeout as much as possible, I've asked in Slack if there are any other alternatives that don't involve using setTimeout. This is just to get a broader perspective before we decide to proceed with setTimeout.

const unsubscribeTransitionEnd = navigation.addListener('transitionEnd', (event) => {
// Prevent firing the prop callback when user is exiting the page.
if (event?.data?.closing) {
return;
}

clearTimeout(timeout);
setDidScreenTransitionEnd(true);
onEntryTransitionEnd?.();
});
Expand All @@ -192,6 +198,7 @@ function ScreenWrapper(
: undefined;

return () => {
clearTimeout(timeout);
unsubscribeTransitionEnd();

if (beforeRemoveSubscription) {
Expand Down
5 changes: 0 additions & 5 deletions src/hooks/useAnimatedHighlightStyle/config.native.ts
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions src/hooks/useAnimatedHighlightStyle/config.ts
View file
Open in desktop

This file was deleted.

79 changes: 61 additions & 18 deletions src/hooks/useAnimatedHighlightStyle/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import React from 'react';
import React, {useState} from 'react';
import {InteractionManager} from 'react-native';
import {Easing, interpolate, interpolateColor, runOnJS, useAnimatedStyle, useSharedValue, withDelay, withSequence, withTiming} from 'react-native-reanimated';
import useScreenWrapperTranstionStatus from '@hooks/useScreenWrapperTransitionStatus';
import useTheme from '@hooks/useTheme';
import CONST from '@src/CONST';
import {DELAY_FACTOR} from './config';

type Props = {
/** Border radius of the wrapper */
Expand All @@ -12,14 +12,26 @@ type Props = {
/** Height of the item that is to be faded */
height: number;

/** Whether the item should be highlighted */
shouldHighlight: boolean;
/** Delay before the highlighted item enters */
itemEnterDelay?: number;

/** Duration in which the item enters */
itemEnterDuration?: number;

/** Delay before the item starts to get highlighted */
highlightStartDelay?: number;

/** Duration in which the item gets fully highlighted */
highlightStartDuration?: number;

/** Delay before the item starts to get un-highlighted */
highlightEndDelay?: number;

/** Duration of the highlight animation */
highlightDuration?: number;
/** Duration in which the item gets fully un-highlighted */
highlightEndDuration?: number;

/** Delay before the highlight animation starts */
delay?: number;
/** Whether the item should be highlighted */
shouldHighlight: boolean;
};

/**
Expand All @@ -28,37 +40,68 @@ type Props = {
export default function useAnimatedHighlightStyle({
borderRadius,
shouldHighlight,
highlightDuration = CONST.ANIMATED_HIGHLIGHT_DURATION,
delay = CONST.ANIMATED_HIGHLIGHT_DELAY,
itemEnterDelay = CONST.ANIMATED_HIGHLIGHT_ENTRY_DELAY,
itemEnterDuration = CONST.ANIMATED_HIGHLIGHT_ENTRY_DURATION,
highlightStartDelay = CONST.ANIMATED_HIGHLIGHT_START_DELAY,
highlightStartDuration = CONST.ANIMATED_HIGHLIGHT_START_DURATION,
highlightEndDelay = CONST.ANIMATED_HIGHLIGHT_END_DELAY,
highlightEndDuration = CONST.ANIMATED_HIGHLIGHT_END_DURATION,
height,
}: Props) {
const actualDelay = delay * DELAY_FACTOR;
const [startHighlight, setStartHighlight] = useState(false);
const repeatableProgress = useSharedValue(0);
const nonRepeatableProgress = useSharedValue(shouldHighlight ? 0 : 1);
const {didScreenTransitionEnd} = useScreenWrapperTranstionStatus();
const theme = useTheme();

const highlightBackgroundStyle = useAnimatedStyle(() => ({
backgroundColor: interpolateColor(repeatableProgress.value, [0, 1], ['rgba(0, 0, 0, 0)', theme.border]),
backgroundColor: interpolateColor(repeatableProgress.value, [0, 1], [theme.appBG, theme.border]),
height: interpolate(nonRepeatableProgress.value, [0, 1], [0, height]),
opacity: interpolate(nonRepeatableProgress.value, [0, 1], [0, 1]),
borderRadius,
}));

React.useEffect(() => {
if (!shouldHighlight) {
if (!shouldHighlight || startHighlight) {
return;
}
setStartHighlight(true);
}, [shouldHighlight, startHighlight]);

React.useEffect(() => {
if (!startHighlight || !didScreenTransitionEnd) {
return;
}
setStartHighlight(false);
InteractionManager.runAfterInteractions(() => {
runOnJS(() => {
nonRepeatableProgress.value = withDelay(actualDelay, withTiming(1, {duration: highlightDuration, easing: Easing.inOut(Easing.ease)}));
repeatableProgress.value = withSequence(
withDelay(actualDelay, withTiming(1, {duration: highlightDuration, easing: Easing.inOut(Easing.ease)})),
withDelay(actualDelay, withTiming(0, {duration: highlightDuration, easing: Easing.inOut(Easing.ease)})),
nonRepeatableProgress.value = withDelay(
itemEnterDelay,
withTiming(1, {duration: itemEnterDuration, easing: Easing.inOut(Easing.ease)}, (finished) => {
if (!finished) {
return;
}

repeatableProgress.value = withSequence(
withDelay(highlightStartDelay, withTiming(1, {duration: highlightStartDuration, easing: Easing.inOut(Easing.ease)})),
withDelay(highlightEndDelay, withTiming(0, {duration: highlightEndDuration, easing: Easing.inOut(Easing.ease)})),
);
}),
);
})();
});
}, [shouldHighlight, highlightDuration, actualDelay, repeatableProgress, nonRepeatableProgress]);
}, [
didScreenTransitionEnd,
startHighlight,
itemEnterDelay,
itemEnterDuration,
highlightStartDelay,
highlightStartDuration,
highlightEndDelay,
highlightEndDuration,
repeatableProgress,
nonRepeatableProgress,
]);

return highlightBackgroundStyle;
}
45 changes: 14 additions & 31 deletions src/libs/actions/Policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ import * as TransactionUtils from '@libs/TransactionUtils';
import type {PolicySelector} from '@pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Route} from '@src/ROUTES';
import ROUTES from '@src/ROUTES';
import type {
InvitedEmailsToAccountIDs,
Expand Down Expand Up @@ -4003,26 +4002,10 @@ function openPolicyDistanceRatesPage(policyID?: string) {
API.read(READ_COMMANDS.OPEN_POLICY_DISTANCE_RATES_PAGE, params);
}

function navigateWhenEnableFeature(policyID: string, featureRoute: Route) {
const isNarrowLayout = getIsNarrowLayout();
if (isNarrowLayout) {
setTimeout(() => {
Navigation.navigate(ROUTES.WORKSPACE_INITIAL.getRoute(policyID));
}, 1000);
return;
}

/**
* The app needs to set a navigation action to the microtask queue, it guarantees to execute Onyx.update first, then the navigation action.
* More details - https://github.com/Expensify/App/issues/37785#issuecomment-1989056726.
*/
new Promise<void>((resolve) => {
resolve();
}).then(() => {
requestAnimationFrame(() => {
Navigation.navigate(featureRoute);
});
});
function navigateWhenEnableFeature(policyID: string) {
setTimeout(() => {
Navigation.navigate(ROUTES.WORKSPACE_INITIAL.getRoute(policyID));
}, CONST.WORKSPACE_ENABLE_FEATURE_REDIRECT_DELAY);
}

function enablePolicyCategories(policyID: string, enabled: boolean) {
Expand Down Expand Up @@ -4068,8 +4051,8 @@ function enablePolicyCategories(policyID: string, enabled: boolean) {

API.write(WRITE_COMMANDS.ENABLE_POLICY_CATEGORIES, parameters, onyxData);

if (enabled) {
navigateWhenEnableFeature(policyID, ROUTES.WORKSPACE_CATEGORIES.getRoute(policyID));
if (enabled && getIsNarrowLayout()) {
navigateWhenEnableFeature(policyID);
}
}

Expand Down Expand Up @@ -4160,8 +4143,8 @@ function enablePolicyDistanceRates(policyID: string, enabled: boolean) {

API.write(WRITE_COMMANDS.ENABLE_POLICY_DISTANCE_RATES, parameters, onyxData);

if (enabled) {
navigateWhenEnableFeature(policyID, ROUTES.WORKSPACE_DISTANCE_RATES.getRoute(policyID));
if (enabled && getIsNarrowLayout()) {
navigateWhenEnableFeature(policyID);
}
}

Expand Down Expand Up @@ -4252,8 +4235,8 @@ function enablePolicyTags(policyID: string, enabled: boolean) {

API.write(WRITE_COMMANDS.ENABLE_POLICY_TAGS, parameters, onyxData);

if (enabled) {
navigateWhenEnableFeature(policyID, ROUTES.WORKSPACE_TAGS.getRoute(policyID));
if (enabled && getIsNarrowLayout()) {
navigateWhenEnableFeature(policyID);
}
}

Expand Down Expand Up @@ -4365,8 +4348,8 @@ function enablePolicyTaxes(policyID: string, enabled: boolean) {
}
API.write(WRITE_COMMANDS.ENABLE_POLICY_TAXES, parameters, onyxData);

if (enabled) {
navigateWhenEnableFeature(policyID, ROUTES.WORKSPACE_TAXES.getRoute(policyID));
if (enabled && getIsNarrowLayout()) {
navigateWhenEnableFeature(policyID);
}
}

Expand Down Expand Up @@ -4456,8 +4439,8 @@ function enablePolicyWorkflows(policyID: string, enabled: boolean) {

API.write(WRITE_COMMANDS.ENABLE_POLICY_WORKFLOWS, parameters, onyxData);

if (enabled) {
navigateWhenEnableFeature(policyID, ROUTES.WORKSPACE_WORKFLOWS.getRoute(policyID));
if (enabled && getIsNarrowLayout()) {
navigateWhenEnableFeature(policyID);
}
}

Expand Down
Loading