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.

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

Replace navigation blur listeners with useFocusEffect #53735

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions src/components/Button/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ type ButtonProps = Partial<ChildrenProps> & {
/** Id to use for this button */
id?: string;

/** Used to locate this button in ui tests */
testID?: string;

/** Accessibility label for the component */
accessibilityLabel?: string;

Expand Down Expand Up @@ -237,6 +240,7 @@ function Button(
shouldShowRightIcon = false,

id = '',
testID = undefined,
CyberAndrii marked this conversation as resolved.
Show resolved Hide resolved
accessibilityLabel = '',
isSplitButton = false,
link = false,
Expand Down Expand Up @@ -405,6 +409,7 @@ function Button(
]}
disabledStyle={disabledStyle}
id={id}
testID={testID}
accessibilityLabel={accessibilityLabel}
role={CONST.ROLE.BUTTON}
hoverDimmingValue={1}
Expand Down
1 change: 1 addition & 0 deletions src/components/ConfirmationPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ function ConfirmationPage({
success
large
text={buttonText}
testID="confirmation-button"
style={styles.mt6}
pressOnEnter
onPress={onButtonPress}
Expand Down
27 changes: 12 additions & 15 deletions src/hooks/useWaitForNavigation.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {useNavigation} from '@react-navigation/native';
import {useEffect, useRef} from 'react';
import {useFocusEffect} from '@react-navigation/native';
import {useCallback, useRef} from 'react';

type UseWaitForNavigation = (navigate: () => void) => () => Promise<void>;

Expand All @@ -8,21 +8,18 @@ type UseWaitForNavigation = (navigate: () => void) => () => Promise<void>;
* Only use when navigating by react-navigation
*/
export default function useWaitForNavigation(): UseWaitForNavigation {
const navigation = useNavigation();
const resolvePromises = useRef<Array<() => void>>([]);

useEffect(() => {
const unsubscribeBlur = navigation.addListener('blur', () => {
resolvePromises.current.forEach((resolve) => {
resolve();
});
resolvePromises.current = [];
});

return () => {
CyberAndrii marked this conversation as resolved.
Show resolved Hide resolved
unsubscribeBlur();
};
}, [navigation]);
useFocusEffect(
useCallback(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@adamgrzybowski @WojtekBoman could you also please review this change in case you could see any issue with it in future?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see anything weird about these changes.

One thing I wonder is why useFocusEffect works and addEventListnere('blur', ()=> {...}) doesn't while useFocusEffect uses the blur event underneath 🤔

https://github.com/react-navigation/react-navigation/blob/2a745c8c598f95fcec5bbf5442045478d4046663/packages/core/src/useFocusEffect.tsx#L95-L101

Copy link
Contributor

Choose a reason for hiding this comment

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

@adamgrzybowski because useFocusEffect runs cleanup on unmount (in native-stack we can't get blur event n that code)

return () => {
CyberAndrii marked this conversation as resolved.
Show resolved Hide resolved
resolvePromises.current.forEach((resolve) => {
resolve();
});
resolvePromises.current = [];
};
}, []),
);

return (navigate: () => void) => () => {
navigate();
Expand Down
1 change: 1 addition & 0 deletions src/pages/workspace/upgrade/UpgradeIntro.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ function UpgradeIntro({feature, onUpgrade, buttonDisabled, loading, isCategorizi
<Button
isLoading={loading}
text={translate('common.upgrade')}
testID="upgrade-button"
success
onPress={onUpgrade}
isDisabled={buttonDisabled}
Expand Down
29 changes: 14 additions & 15 deletions src/pages/workspace/upgrade/WorkspaceUpgradePage.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {useNavigation} from '@react-navigation/native';
import React, {useCallback, useEffect} from 'react';
import {useFocusEffect} from '@react-navigation/native';
import React, {useCallback, useMemo} from 'react';
import {useOnyx} from 'react-native-onyx';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import ScreenWrapper from '@components/ScreenWrapper';
Expand Down Expand Up @@ -37,20 +37,19 @@ function getFeatureNameAlias(featureName: string) {
}

function WorkspaceUpgradePage({route}: WorkspaceUpgradePageProps) {
const navigation = useNavigation();
const styles = useThemeStyles();
const policyID = route.params.policyID;

const featureNameAlias = getFeatureNameAlias(route.params.featureName);

const feature = Object.values(CONST.UPGRADE_FEATURE_INTRO_MAPPING).find((f) => f.alias === featureNameAlias);
const feature = useMemo(() => Object.values(CONST.UPGRADE_FEATURE_INTRO_MAPPING).find((f) => f.alias === featureNameAlias), [featureNameAlias]);
const {translate} = useLocalize();
const [policy] = useOnyx(`policy_${policyID}`);
const qboConfig = policy?.connections?.quickbooksOnline?.config;
const {isOffline} = useNetwork();

const canPerformUpgrade = !!feature && !!policy && PolicyUtils.isPolicyAdmin(policy);
const isUpgraded = React.useMemo(() => PolicyUtils.isControlPolicy(policy), [policy]);
const isUpgraded = useMemo(() => PolicyUtils.isControlPolicy(policy), [policy]);

const perDiemCustomUnit = PolicyUtils.getPerDiemCustomUnit(policy);
const categoryId = route.params?.categoryId;
Expand Down Expand Up @@ -155,16 +154,16 @@ function WorkspaceUpgradePage({route}: WorkspaceUpgradePageProps) {
route.params.featureName,
]);

useEffect(() => {
const unsubscribeListener = navigation.addListener('blur', () => {
if (!isUpgraded || !canPerformUpgrade) {
return;
}
confirmUpgrade();
});

return unsubscribeListener;
}, [isUpgraded, canPerformUpgrade, confirmUpgrade, navigation]);
useFocusEffect(
useCallback(() => {
return () => {
if (!isUpgraded || !canPerformUpgrade) {
return;
}
confirmUpgrade();
};
}, [isUpgraded, canPerformUpgrade, confirmUpgrade]),
);

if (!canPerformUpgrade) {
return <NotFoundPage />;
Expand Down
76 changes: 76 additions & 0 deletions tests/ui/WorkspaceUpgradeTest.tsx
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, great job adding such test!

Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/* eslint-disable @typescript-eslint/naming-convention */
import {NavigationContainer} from '@react-navigation/native';
import {act, fireEvent, render, screen} from '@testing-library/react-native';
import React from 'react';
import Onyx from 'react-native-onyx';
import {WRITE_COMMANDS} from '@libs/API/types';
import * as SequentialQueue from '@libs/Network/SequentialQueue';
import createResponsiveStackNavigator from '@navigation/AppNavigator/createResponsiveStackNavigator';
import type {SettingsNavigatorParamList} from '@navigation/types';
import WorkspaceUpgradePage from '@pages/workspace/upgrade/WorkspaceUpgradePage';
import ONYXKEYS from '@src/ONYXKEYS';
import SCREENS from '@src/SCREENS';
import type {Policy} from '@src/types/onyx';
import * as LHNTestUtils from '../utils/LHNTestUtils';
import * as TestHelper from '../utils/TestHelper';
import waitForBatchedUpdates from '../utils/waitForBatchedUpdates';
import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct';

TestHelper.setupGlobalFetchMock();

const RootStack = createResponsiveStackNavigator<SettingsNavigatorParamList>();

const renderPage = (initialRouteName: typeof SCREENS.WORKSPACE.UPGRADE, initialParams: SettingsNavigatorParamList[typeof SCREENS.WORKSPACE.UPGRADE]) => {
return render(
<NavigationContainer>
<RootStack.Navigator initialRouteName={initialRouteName}>
<RootStack.Screen
name={SCREENS.WORKSPACE.UPGRADE}
component={WorkspaceUpgradePage}
initialParams={initialParams}
/>
</RootStack.Navigator>
</NavigationContainer>,
);
};

describe('WorkspaceUpgrade', () => {
beforeAll(() => {
Onyx.init({
keys: ONYXKEYS,
});
});

afterEach(async () => {
await SequentialQueue.waitForIdle();
await act(async () => {
await Onyx.clear();
});

jest.clearAllMocks();
});

it('should enable policy rules', async () => {
const policy: Policy = LHNTestUtils.getFakePolicy();

// Given that a policy is initialized in Onyx
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, policy);

// And WorkspaceUpgradePage for rules is opened
const {unmount} = renderPage(SCREENS.WORKSPACE.UPGRADE, {policyID: policy.id, featureName: 'rules'});

// When the policy is upgraded by clicking on the Upgrade button
fireEvent.press(screen.getByTestId('upgrade-button'));
await waitForBatchedUpdatesWithAct();

// Then "Upgrade to Corporate" API request should be made
TestHelper.expectAPICommandToHaveBeenCalled(WRITE_COMMANDS.UPGRADE_TO_CORPORATE, 1);

// When WorkspaceUpgradePage is unmounted
unmount();
await waitForBatchedUpdates();

// Then "Set policy rules enabled" API request should be made
TestHelper.expectAPICommandToHaveBeenCalled(WRITE_COMMANDS.SET_POLICY_RULES_ENABLED, 1);
});
});
Loading