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

User can access the background list using the arrow keys while the RHP is open #39327

Merged
merged 14 commits into from
Apr 5, 2024
Merged

User can access the background list using the arrow keys while the RHP is open #39327

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b89c911
Fix bug with User can access the background list using the arrow key…
ZhenjaHorbach Mar 30, 2024
b771515
Refactor ArrowKeyFocusManager
ZhenjaHorbach Mar 30, 2024
108f04f
Update propTypes for ArrowKeyFocusManager
ZhenjaHorbach Mar 30, 2024
e3f803b
Update propTypes for ArrowKeyFocusManager x2
ZhenjaHorbach Mar 30, 2024
9efe712
Update perf-tests
ZhenjaHorbach Mar 30, 2024
9eba416
Update perf-tests x2
ZhenjaHorbach Mar 30, 2024
defd731
Update perf-tests x3
ZhenjaHorbach Mar 30, 2024
aeb387f
Revert some changes
ZhenjaHorbach Mar 30, 2024
aa60c7e
Revert some changes x2
ZhenjaHorbach Mar 30, 2024
13affe7
Update perf-tests
ZhenjaHorbach Mar 30, 2024
aae8ff4
Update perf-tests x2
ZhenjaHorbach Mar 30, 2024
917cf00
Update perf-tests x3
ZhenjaHorbach Mar 30, 2024
f4f7b19
Fix bug with blue border
ZhenjaHorbach Mar 31, 2024
2f1cede
Update branch
ZhenjaHorbach Apr 4, 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
29 changes: 23 additions & 6 deletions src/components/ArrowKeyFocusManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {useIsFocused} from '@react-navigation/native';
import PropTypes from 'prop-types';
import {Component} from 'react';
import React, {Component} from 'react';
import KeyboardShortcut from '@libs/KeyboardShortcut';
import CONST from '@src/CONST';

Expand All @@ -16,6 +17,9 @@ const propTypes = {
/** The maximum index – provided so that the focus can be sent back to the beginning of the list when the end is reached. */
maxIndex: PropTypes.number.isRequired,

/** Whether navigation is focused */
isFocused: PropTypes.bool.isRequired,

/** A callback executed when the focused input changes. */
onFocusedIndexChanged: PropTypes.func.isRequired,

Expand All @@ -32,7 +36,7 @@ const defaultProps = {
shouldResetIndexOnEndReached: true,
};

class ArrowKeyFocusManager extends Component {
class BaseArrowKeyFocusManager extends Component {
componentDidMount() {
const arrowUpConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_UP;
const arrowDownConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN;
Expand Down Expand Up @@ -77,7 +81,7 @@ class ArrowKeyFocusManager extends Component {
}

onArrowUpKey() {
if (this.props.maxIndex < 0) {
if (this.props.maxIndex < 0 || !this.props.isFocused) {
return;
}

Expand All @@ -96,7 +100,7 @@ class ArrowKeyFocusManager extends Component {
}

onArrowDownKey() {
if (this.props.maxIndex < 0) {
if (this.props.maxIndex < 0 || !this.props.isFocused) {
return;
}

Expand All @@ -119,7 +123,20 @@ class ArrowKeyFocusManager extends Component {
}
}

ArrowKeyFocusManager.propTypes = propTypes;
ArrowKeyFocusManager.defaultProps = defaultProps;
function ArrowKeyFocusManager(props) {
const isFocused = useIsFocused();

return (
<BaseArrowKeyFocusManager
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
isFocused={isFocused}
/>
);
}

BaseArrowKeyFocusManager.propTypes = propTypes;
BaseArrowKeyFocusManager.defaultProps = defaultProps;
ArrowKeyFocusManager.displayName = 'ArrowKeyFocusManager';
Copy link
Contributor

Choose a reason for hiding this comment

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

@ZhenjaHorbach We should create a folder with the name ArrowKeyFocusManager, and under this folder, BaseArrowKeyFocusManager.js and index.js files.

ArrowKeyFocusManager
    BaseArrowKeyFocusManager.js
    index.js

Copy link
Contributor Author

@ZhenjaHorbach ZhenjaHorbach Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this doesn’t make sense, since we have a rule that prohibits creating new js files )
And for ts files we have separate tickets )

Copy link
Contributor

Choose a reason for hiding this comment

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

I was searching for GH issues and wondering if is there any ticket was created to replace ArrowKeyFocusManager with useArrowKeyFocusManager usage For SelectionList

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw this comment today, and If ArrowKeyFocusManager is going to be removed, then we should not add any logic to the ArrowKeyFocusManager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, maybe we should remove ArrowKeyFocusManage from the lists?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then let's move forward with this PR 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Or are you afraid that the PR will undo the changes we are doing here?

@pecanoro Yes, I was thinking that once that PR is merged, then SelectionList will no longer use ArrowKeyFocusManager and In current PR, We are adding changes to the ArrowKeyFocusManager Component and these changes will not be in effect, and also ArrowKeyFocusManager usage will be removed entirely soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's finish reviewing this PR and I will post in the other issue to let them know about the changes here. But at this point I am not sure which one is closer to get merged 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, the PR is ready to review )

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I would say I'll complete the checklist in the next hour 😃


export default ArrowKeyFocusManager;
2 changes: 2 additions & 0 deletions src/pages/workspace/categories/WorkspaceCategoriesPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import {deleteWorkspaceCategories, setWorkspaceCategoryEnabled} from '@libs/actions/Policy';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import localeCompare from '@libs/LocaleCompare';
import Navigation from '@libs/Navigation/Navigation';
import * as PolicyUtils from '@libs/PolicyUtils';
Expand Down Expand Up @@ -316,6 +317,7 @@ function WorkspaceCategoriesPage({policy, policyCategories, route}: WorkspaceCat
sections={[{data: categoryList, isDisabled: false}]}
onCheckboxPress={toggleCategory}
onSelectRow={navigateToCategorySettings}
shouldPreventDefaultFocusOnSelectRow={!DeviceCapabilities.canUseTouchScreen()}
onSelectAll={toggleAllCategories}
showScrollIndicator
ListItem={TableListItem}
Expand Down
2 changes: 2 additions & 0 deletions src/pages/workspace/distanceRates/PolicyDistanceRatesPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import * as CurrencyUtils from '@libs/CurrencyUtils';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import Navigation from '@libs/Navigation/Navigation';
import type {WorkspacesCentralPaneNavigatorParamList} from '@navigation/types';
import AdminPolicyAccessOrNotFoundWrapper from '@pages/workspace/AdminPolicyAccessOrNotFoundWrapper';
Expand Down Expand Up @@ -308,6 +309,7 @@ function PolicyDistanceRatesPage({policy, route}: PolicyDistanceRatesPageProps)
onDismissError={dismissError}
showScrollIndicator
ListItem={TableListItem}
shouldPreventDefaultFocusOnSelectRow={!DeviceCapabilities.canUseTouchScreen()}
customListHeader={getCustomListHeader()}
listHeaderWrapperStyle={[styles.ph9, styles.pv3, styles.pb5]}
/>
Expand Down
2 changes: 2 additions & 0 deletions src/pages/workspace/tags/WorkspaceTagsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import useNetwork from '@hooks/useNetwork';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import localeCompare from '@libs/LocaleCompare';
import Navigation from '@libs/Navigation/Navigation';
import * as PolicyUtils from '@libs/PolicyUtils';
Expand Down Expand Up @@ -315,6 +316,7 @@ function WorkspaceTagsPage({policyTags, route}: WorkspaceTagsPageProps) {
showScrollIndicator
ListItem={TableListItem}
customListHeader={getCustomListHeader()}
shouldPreventDefaultFocusOnSelectRow={!DeviceCapabilities.canUseTouchScreen()}
listHeaderWrapperStyle={[styles.ph9, styles.pv3, styles.pb5]}
onDismissError={(item) => Policy.clearPolicyTagErrors(route.params.policyID, item.value)}
/>
Expand Down
2 changes: 2 additions & 0 deletions src/pages/workspace/taxes/WorkspaceTaxesPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import {openPolicyTaxesPage} from '@libs/actions/Policy';
import {clearTaxRateError, deletePolicyTaxes, setPolicyTaxesEnabled} from '@libs/actions/TaxRate';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import * as ErrorUtils from '@libs/ErrorUtils';
import Navigation from '@libs/Navigation/Navigation';
import * as PolicyUtils from '@libs/PolicyUtils';
Expand Down Expand Up @@ -276,6 +277,7 @@ function WorkspaceTaxesPage({
showScrollIndicator
ListItem={TableListItem}
customListHeader={getCustomListHeader()}
shouldPreventDefaultFocusOnSelectRow={!DeviceCapabilities.canUseTouchScreen()}
listHeaderWrapperStyle={[styles.ph9, styles.pv3, styles.pb5]}
onDismissError={(item) => (item.keyForList ? clearTaxRateError(policyID, item.keyForList, item.pendingAction) : undefined)}
/>
Expand Down
13 changes: 13 additions & 0 deletions tests/perf-test/OptionsSelector.perf-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type {ComponentType} from 'react';
import {measurePerformance} from 'reassure';
import type {WithLocalizeProps} from '@components/withLocalize';
import type {WithNavigationFocusProps} from '@components/withNavigationFocus';
import type Navigation from '@libs/Navigation/Navigation';
import OptionsSelector from '@src/components/OptionsSelector';
import variables from '@src/styles/variables';

Expand Down Expand Up @@ -38,6 +39,18 @@ jest.mock('@src/components/withNavigationFocus', () => (Component: ComponentType
return WithNavigationFocus;
});

jest.mock('@react-navigation/native', () => {
const actualNav = jest.requireActual('@react-navigation/native');
return {
...actualNav,
useNavigation: () => ({
navigate: jest.fn(),
addListener: () => jest.fn(),
}),
useIsFocused: () => true,
} as typeof Navigation;
});

type GenerateSectionsProps = Array<{numberOfItems: number; shouldShow?: boolean}>;

const generateSections = (sections: GenerateSectionsProps) =>
Expand Down
4 changes: 1 addition & 3 deletions tests/perf-test/ReportActionCompose.perf-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,7 @@ jest.mock('@react-navigation/native', () => {
navigate: jest.fn(),
addListener: () => jest.fn(),
}),
useIsFocused: () => ({
navigate: jest.fn(),
}),
useIsFocused: () => true,
} as typeof Navigation;
});

Expand Down
5 changes: 1 addition & 4 deletions tests/perf-test/ReportScreen.perf-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,12 @@ jest.mock('@src/hooks/usePermissions.ts');

jest.mock('@src/libs/Navigation/Navigation');

const mockedNavigate = jest.fn();
jest.mock('@react-navigation/native', () => {
const actualNav = jest.requireActual('@react-navigation/native');
return {
...actualNav,
useFocusEffect: jest.fn(),
useIsFocused: () => ({
navigate: mockedNavigate,
}),
useIsFocused: () => true,
useRoute: () => jest.fn(),
useNavigation: () => ({
navigate: jest.fn(),
Expand Down
5 changes: 1 addition & 4 deletions tests/perf-test/SearchPage.perf-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,12 @@ jest.mock('@src/libs/API', () => ({

jest.mock('@src/libs/Navigation/Navigation');

const mockedNavigate = jest.fn();
jest.mock('@react-navigation/native', () => {
const actualNav = jest.requireActual('@react-navigation/native');
return {
...actualNav,
useFocusEffect: jest.fn(),
useIsFocused: () => ({
navigate: mockedNavigate,
}),
useIsFocused: () => true,
useRoute: () => jest.fn(),
useNavigation: () => ({
navigate: jest.fn(),
Expand Down
5 changes: 1 addition & 4 deletions tests/perf-test/SignInPage.perf-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,12 @@ jest.mock('../../src/libs/API', () => ({
read: jest.fn(),
}));

const mockedNavigate = jest.fn();
jest.mock('@react-navigation/native', () => {
const actualNav = jest.requireActual('@react-navigation/native');
return {
...actualNav,
useFocusEffect: jest.fn(),
useIsFocused: () => ({
navigate: mockedNavigate,
}),
useIsFocused: () => true,
useRoute: () => jest.fn(),
useNavigation: () => ({
navigate: jest.fn(),
Expand Down
8 changes: 1 addition & 7 deletions tests/utils/LHNTestUtils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
/* eslint-disable @typescript-eslint/naming-convention */
import type {NavigationProp} from '@react-navigation/core/src/types';
import type * as Navigation from '@react-navigation/native';
import type {ParamListBase} from '@react-navigation/routers';
import {render} from '@testing-library/react-native';
import type {ReactElement} from 'react';
import React from 'react';
Expand Down Expand Up @@ -33,17 +31,13 @@ type MockedSidebarLinksProps = {
currentReportID?: string;
};

// we have to mock `useIsFocused` because it's used in the SidebarLinks component
const mockedNavigate: jest.MockedFn<NavigationProp<ParamListBase>['navigate']> = jest.fn();
jest.mock('@react-navigation/native', (): typeof Navigation => {
const actualNav = jest.requireActual('@react-navigation/native');
return {
...actualNav,
useRoute: jest.fn(),
useFocusEffect: jest.fn(),
useIsFocused: () => ({
navigate: mockedNavigate,
}),
useIsFocused: () => true,
useNavigation: () => ({
navigate: jest.fn(),
addListener: jest.fn(),
Expand Down
Loading