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

fix: ignore other actions if navigation in progress in menu Items #28338

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
10922f7
fix: ignore other actions if navigation in progress in worksspace set…
ishpaul777 Sep 27, 2023
d4fef7a
Merge branch 'Expensify:main' into fix/quickly-pressing-different-men…
ishpaul777 Oct 9, 2023
76ef509
added fix for all security, workspace and aboutpage
ishpaul777 Oct 9, 2023
7d8fda9
remove console log
ishpaul777 Oct 9, 2023
6c90c16
fix linters
ishpaul777 Oct 9, 2023
31abad1
added requested changes
ishpaul777 Oct 10, 2023
c99fd1c
added comment why execption made for mapping menuitems
ishpaul777 Oct 10, 2023
092fd1b
resolve conflicts
ishpaul777 Oct 12, 2023
7dc1771
Merge branch 'main' into fix/quickly-pressing-different-menuitems
ishpaul777 Oct 12, 2023
31ca2f8
fixed some more conflicts
ishpaul777 Oct 12, 2023
2dfd0ea
Merge branch 'main' into fix/quickly-pressing-different-menuitems
ishpaul777 Oct 16, 2023
64ed5e6
fix unneccessary prop passing to backbutton
ishpaul777 Oct 16, 2023
89a1320
fix linters
ishpaul777 Oct 16, 2023
2f81e3b
cleaning up the code
ishpaul777 Oct 17, 2023
d12e538
Merge branch 'main' into fix/quickly-pressing-different-menuitems
ishpaul777 Oct 17, 2023
3ce0a1b
Update src/components/MenuItemList.js
ishpaul777 Oct 17, 2023
aa5a1c7
Update src/components/HeaderWithBackButton/headerWithBackButtonPropTy…
ishpaul777 Oct 17, 2023
dbc44ae
added suggested changes and fix hide of assistance button when delete…
ishpaul777 Oct 17, 2023
1832899
Update src/components/HeaderWithBackButton/headerWithBackButtonPropTy…
ishpaul777 Oct 18, 2023
689fe1d
Update src/components/HeaderWithBackButton/headerWithBackButtonPropTy…
ishpaul777 Oct 18, 2023
0bb8866
Update src/pages/workspace/WorkspaceInitialPage.js
ishpaul777 Oct 18, 2023
6c220e6
fix menu item blinking when modal is visible
ishpaul777 Oct 18, 2023
e665fbf
Merge branch 'fix/quickly-pressing-different-menuitems' of https://gi…
ishpaul777 Oct 18, 2023
38e6e69
Merge branch 'main' into fix/quickly-pressing-different-menuitems
ishpaul777 Oct 20, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,18 @@ const propTypes = {
/** Whether we should show a get assistance (question mark) button */
shouldShowGetAssistanceButton: PropTypes.bool,

/** Whether we should disable the get assistance button */
shouldDisableGetAssistanceButton: PropTypes.bool,

/** Whether we should show a pin button */
shouldShowPinButton: PropTypes.bool,

/** Whether we should show a more options (threedots) button */
shouldShowThreeDotsButton: PropTypes.bool,

/** Whether we should disable threedots button */
shouldDisableThreeDotsButton: PropTypes.bool,

/** List of menu items for more(three dots) menu */
threeDotsMenuItems: ThreeDotsMenuItemPropTypes,

Expand Down Expand Up @@ -84,6 +90,9 @@ const propTypes = {

/** Children to wrap in Header */
children: PropTypes.node,

/** Single execution function to prevent concurrent navigation actions */
singleExecution: PropTypes.func,
};

export default propTypes;
9 changes: 8 additions & 1 deletion src/components/HeaderWithBackButton/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import headerWithBackButtonPropTypes from './headerWithBackButtonPropTypes';
import useThrottledButtonState from '../../hooks/useThrottledButtonState';
import useLocalize from '../../hooks/useLocalize';
import useKeyboardState from '../../hooks/useKeyboardState';
import useWaitForNavigation from '../../hooks/useWaitForNavigation';

function HeaderWithBackButton({
iconFill = undefined,
Expand All @@ -35,8 +36,10 @@ function HeaderWithBackButton({
shouldShowCloseButton = false,
shouldShowDownloadButton = false,
shouldShowGetAssistanceButton = false,
shouldDisableGetAssistanceButton = false,
shouldShowPinButton = false,
shouldShowThreeDotsButton = false,
shouldDisableThreeDotsButton = false,
stepCounter = null,
subtitle = '',
title = '',
Expand All @@ -49,10 +52,12 @@ function HeaderWithBackButton({
shouldEnableDetailPageNavigation = false,
children = null,
shouldOverlay = false,
singleExecution = (func) => func,
}) {
const [isDownloadButtonActive, temporarilyDisableDownloadButton] = useThrottledButtonState();
const {translate} = useLocalize();
const {isKeyboardShown} = useKeyboardState();
const waitForNavigate = useWaitForNavigation();
Copy link
Contributor

Choose a reason for hiding this comment

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

👋 coming from #30636

The hooks here cause an error in the storybook for component HeaderWithBackButton.

return (
<View style={[styles.headerBar, shouldShowBorderBottom && styles.borderBottom, shouldShowBackButton && styles.pl2]}>
<View style={[styles.dFlex, styles.flexRow, styles.alignItemsCenter, styles.flexGrow1, styles.justifyContentBetween, styles.overflowHidden]}>
Expand Down Expand Up @@ -121,7 +126,8 @@ function HeaderWithBackButton({
{shouldShowGetAssistanceButton && (
<Tooltip text={translate('getAssistancePage.questionMarkButtonTooltip')}>
<PressableWithoutFeedback
onPress={() => Navigation.navigate(ROUTES.GET_ASSISTANCE.getRoute(guidesCallTaskID))}
disabled={shouldDisableGetAssistanceButton}
onPress={singleExecution(waitForNavigate(() => Navigation.navigate(ROUTES.GET_ASSISTANCE.getRoute(guidesCallTaskID))))}
style={[styles.touchableButtonImage]}
accessibilityRole="button"
accessibilityLabel={translate('getAssistancePage.questionMarkButtonTooltip')}
Expand All @@ -136,6 +142,7 @@ function HeaderWithBackButton({
{shouldShowPinButton && <PinButton report={report} />}
{shouldShowThreeDotsButton && (
<ThreeDotsMenu
disabled={shouldDisableThreeDotsButton}
menuItems={threeDotsMenuItems}
onIconPress={onThreeDotsButtonPress}
anchorPosition={threeDotsAnchorPosition}
Expand Down
8 changes: 8 additions & 0 deletions src/components/MenuItemList.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';
import _ from 'underscore';
import PropTypes from 'prop-types';
import useSingleExecution from '../hooks/useSingleExecution';
import MenuItem from './MenuItem';
import menuItemPropTypes from './menuItemPropTypes';
import * as ReportActionContextMenu from '../pages/home/report/ContextMenu/ReportActionContextMenu';
Expand All @@ -9,13 +10,18 @@ import {CONTEXT_MENU_TYPES} from '../pages/home/report/ContextMenu/ContextMenuAc
const propTypes = {
/** An array of props that are pass to individual MenuItem components */
menuItems: PropTypes.arrayOf(PropTypes.shape(menuItemPropTypes)),

/** Whether or not to use the single execution hook */
ishpaul777 marked this conversation as resolved.
Show resolved Hide resolved
shouldUseSingleExecution: PropTypes.bool,
};
const defaultProps = {
menuItems: [],
shouldUseSingleExecution: false,
};

function MenuItemList(props) {
let popoverAnchor;
const {isExecuting, singleExecution} = useSingleExecution();

/**
* Handle the secondary interaction for a menu item.
Expand All @@ -41,6 +47,8 @@ function MenuItemList(props) {
shouldBlockSelection={Boolean(menuItemProps.link)}
// eslint-disable-next-line react/jsx-props-no-spreading
{...menuItemProps}
disabled={menuItemProps.disabled || isExecuting}
onPress={props.shouldUseSingleExecution ? singleExecution(menuItemProps.onPress) : menuItemProps.onPress}
/>
))}
</>
Expand Down
7 changes: 6 additions & 1 deletion src/components/ThreeDotsMenu/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,16 @@ const propTypes = {
/** Whether the popover menu should overlay the current view */
shouldOverlay: PropTypes.bool,

/** Whether the menu is disabled */
disabled: PropTypes.bool,

/** Should we announce the Modal visibility changes? */
shouldSetModalVisibility: PropTypes.bool,
};

const defaultProps = {
iconTooltip: 'common.more',
disabled: false,
iconFill: undefined,
iconStyles: [],
icon: Expensicons.ThreeDots,
Expand All @@ -68,7 +72,7 @@ const defaultProps = {
shouldSetModalVisibility: true,
};

function ThreeDotsMenu({iconTooltip, icon, iconFill, iconStyles, onIconPress, menuItems, anchorPosition, anchorAlignment, shouldOverlay, shouldSetModalVisibility}) {
function ThreeDotsMenu({iconTooltip, icon, iconFill, iconStyles, onIconPress, menuItems, anchorPosition, anchorAlignment, shouldOverlay, shouldSetModalVisibility, disabled}) {
const [isPopupMenuVisible, setPopupMenuVisible] = useState(false);
const buttonRef = useRef(null);
const {translate} = useLocalize();
Expand Down Expand Up @@ -96,6 +100,7 @@ function ThreeDotsMenu({iconTooltip, icon, iconFill, iconStyles, onIconPress, me
onIconPress();
}
}}
disabled={disabled}
onMouseDown={(e) => {
/* Keep the focus state on mWeb like we did on the native apps. */
if (!Browser.isMobile()) {
Expand Down
107 changes: 54 additions & 53 deletions src/pages/settings/AboutPage/AboutPage.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import _ from 'underscore';
import React from 'react';
import React, {useRef, useMemo} from 'react';
import {ScrollView, View} from 'react-native';
import DeviceInfo from 'react-native-device-info';
import HeaderWithBackButton from '../../../components/HeaderWithBackButton';
Expand All @@ -13,7 +13,6 @@ import * as Expensicons from '../../../components/Icon/Expensicons';
import ScreenWrapper from '../../../components/ScreenWrapper';
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions';
import MenuItem from '../../../components/MenuItem';
import Logo from '../../../../assets/images/new-expensify.svg';
import pkg from '../../../../package.json';
import * as Report from '../../../libs/actions/Report';
Expand All @@ -22,6 +21,8 @@ import compose from '../../../libs/compose';
import * as ReportActionContextMenu from '../../home/report/ContextMenu/ReportActionContextMenu';
import {CONTEXT_MENU_TYPES} from '../../home/report/ContextMenu/ContextMenuActions';
import * as Environment from '../../../libs/Environment/Environment';
import MenuItemList from '../../../components/MenuItemList';
import useWaitForNavigation from '../../../hooks/useWaitForNavigation';

const propTypes = {
...withLocalizePropTypes,
Expand All @@ -40,46 +41,57 @@ function getFlavor() {
}

function AboutPage(props) {
let popoverAnchor;
const menuItems = [
{
translationKey: 'initialSettingsPage.aboutPage.appDownloadLinks',
icon: Expensicons.Link,
action: () => {
Navigation.navigate(ROUTES.SETTINGS_APP_DOWNLOAD_LINKS);
const {translate} = props;
const popoverAnchor = useRef(null);
const waitForNavigate = useWaitForNavigation();

const menuItems = useMemo(() => {
const baseMenuItems = [
{
translationKey: 'initialSettingsPage.aboutPage.appDownloadLinks',
icon: Expensicons.Link,
action: waitForNavigate(() => Navigation.navigate(ROUTES.SETTINGS_APP_DOWNLOAD_LINKS)),
},
{
translationKey: 'initialSettingsPage.aboutPage.viewKeyboardShortcuts',
icon: Expensicons.Keyboard,
action: waitForNavigate(() => Navigation.navigate(ROUTES.KEYBOARD_SHORTCUTS)),
},
},
{
translationKey: 'initialSettingsPage.aboutPage.viewKeyboardShortcuts',
icon: Expensicons.Keyboard,
action: () => {
Navigation.navigate(ROUTES.KEYBOARD_SHORTCUTS);
{
translationKey: 'initialSettingsPage.aboutPage.viewTheCode',
icon: Expensicons.Eye,
iconRight: Expensicons.NewWindow,
action: () => {
Link.openExternalLink(CONST.GITHUB_URL);
},
Copy link
Contributor

@abdulrahuman5196 abdulrahuman5196 Jan 11, 2024

Choose a reason for hiding this comment

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

link for github url is missed here during refractor

Causing this issue - #32965

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No excuses.. i messed up here😅

},
},
{
translationKey: 'initialSettingsPage.aboutPage.viewTheCode',
icon: Expensicons.Eye,
iconRight: Expensicons.NewWindow,
action: () => {
Link.openExternalLink(CONST.GITHUB_URL);
{
translationKey: 'initialSettingsPage.aboutPage.viewOpenJobs',
icon: Expensicons.MoneyBag,
iconRight: Expensicons.NewWindow,
action: () => {
Link.openExternalLink(CONST.UPWORK_URL);
},
link: CONST.UPWORK_URL,
},
link: CONST.GITHUB_URL,
},
{
translationKey: 'initialSettingsPage.aboutPage.viewOpenJobs',
icon: Expensicons.MoneyBag,
iconRight: Expensicons.NewWindow,
action: () => {
Link.openExternalLink(CONST.UPWORK_URL);
{
translationKey: 'initialSettingsPage.aboutPage.reportABug',
icon: Expensicons.Bug,
action: waitForNavigate(Report.navigateToConciergeChat),
},
link: CONST.UPWORK_URL,
},
{
translationKey: 'initialSettingsPage.aboutPage.reportABug',
icon: Expensicons.Bug,
action: Report.navigateToConciergeChat,
},
];
];
return _.map(baseMenuItems, (item) => ({
key: item.translationKey,
title: translate(item.translationKey),
icon: item.icon,
iconRight: item.iconRight,
onPress: item.action,
shouldShowRightIcon: true,
onSecondaryInteraction: !_.isEmpty(item.link) ? (e) => ReportActionContextMenu.showContextMenu(CONTEXT_MENU_TYPES.LINK, e, item.link, popoverAnchor) : undefined,
ref: popoverAnchor,
shouldBlockSelection: Boolean(item.link),
}));
}, [translate, waitForNavigate]);

return (
<ScreenWrapper
Expand Down Expand Up @@ -109,21 +121,10 @@ function AboutPage(props) {
<Text style={[styles.baseFontStyle, styles.mv5]}>{props.translate('initialSettingsPage.aboutPage.description')}</Text>
</View>
</View>
{_.map(menuItems, (item) => (
<MenuItem
key={item.translationKey}
title={props.translate(item.translationKey)}
icon={item.icon}
iconRight={item.iconRight}
onPress={() => item.action()}
shouldBlockSelection={Boolean(item.link)}
onSecondaryInteraction={
!_.isEmpty(item.link) ? (e) => ReportActionContextMenu.showContextMenu(CONTEXT_MENU_TYPES.LINK, e, item.link, popoverAnchor) : undefined
}
ref={(el) => (popoverAnchor = el)}
shouldShowRightIcon
/>
))}
<MenuItemList
menuItems={menuItems}
shouldUseSingleExecution
/>
</View>
<View style={[styles.sidebarFooter]}>
<Text
Expand Down
6 changes: 4 additions & 2 deletions src/pages/settings/InitialSettingsPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,8 @@ function InitialSettingsPage(props) {
<Tooltip text={translate('common.profile')}>
<PressableWithoutFeedback
style={styles.mb3}
onPress={openProfileSettings}
disabled={isExecuting}
onPress={singleExecution(openProfileSettings)}
ishpaul777 marked this conversation as resolved.
Show resolved Hide resolved
accessibilityLabel={translate('common.profile')}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
>
Expand All @@ -357,7 +358,8 @@ function InitialSettingsPage(props) {
</Tooltip>
<PressableWithoutFeedback
style={[styles.mt1, styles.mw100]}
onPress={openProfileSettings}
disabled={isExecuting}
onPress={singleExecution(openProfileSettings)}
ishpaul777 marked this conversation as resolved.
Show resolved Hide resolved
accessibilityLabel={translate('common.profile')}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.LINK}
>
Expand Down
Loading
Loading