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: when deleting a workspace offline, the layout breaks #17639

Merged
merged 21 commits into from
Apr 24, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6930885
avoid nested Pressable and change the root MenuItem component, with s…
akinwale Apr 11, 2023
9a094e3
use React.forwardRef for MenuItem, and fix prop types
akinwale Apr 11, 2023
e8e90e1
changes based on PR review feedback
akinwale Apr 12, 2023
d07e3b1
clean up onPressOut in MenuItem component
akinwale Apr 12, 2023
cfb3b76
implement Copy to Clipboard behaviour for all links
akinwale Apr 12, 2023
0240f90
cleanup
akinwale Apr 12, 2023
4da28a3
cleanup boolean checks
akinwale Apr 13, 2023
732c034
update more links with the Copy to Clipboard behaviour
akinwale Apr 13, 2023
3e532f7
conditional onSecondaryInteraction
akinwale Apr 13, 2023
58a96ff
conditional onLongPress
akinwale Apr 13, 2023
4ca7979
cleanup conditional checks, make onSecondaryInteraction prop type opt…
akinwale Apr 13, 2023
75ab457
make executeSecondaryInteraction a class method, fix typo
akinwale Apr 13, 2023
9f61f84
add JSDoc for executeSecondaryInteraction
akinwale Apr 13, 2023
59bcbd9
fix onSecondaryInteraction prop check in MenuItemList
akinwale Apr 13, 2023
3bcbdfb
set defaultProps for MenuItem wrapped with HOC
akinwale Apr 18, 2023
cd1f55c
make applyStrikethrough work with titleStyle
akinwale Apr 19, 2023
2d898b1
fix AboutPage.js merge conflict
akinwale Apr 20, 2023
ae1ae33
conditionally apply styles.offlineFeedback.deleted for menu item text
akinwale Apr 20, 2023
9f9d54b
fix MenuItem.js merge conflict
akinwale Apr 22, 2023
b56477e
code cleanup
akinwale Apr 24, 2023
56a90a7
fix AboutPage.js merge conflict
akinwale Apr 24, 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
1 change: 1 addition & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -2224,6 +2224,7 @@ const CONST = {
'ILS', 'JPY', 'MYR', 'MXN', 'TWD', 'NZD', 'NOK', 'PHP',
'PLN', 'GBP', 'RUB', 'SGD', 'SEK', 'CHF', 'THB', 'USD',
],

CONCIERGE_TRAVEL_URL: 'https://community.expensify.com/discussion/7066/introducing-concierge-travel',
};

Expand Down
28 changes: 22 additions & 6 deletions src/components/MenuItem.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import _ from 'underscore';
import React from 'react';
import {
View, Pressable,
} from 'react-native';
import {View} from 'react-native';
import Text from './Text';
import styles from '../styles/styles';
import * as StyleUtils from '../styles/StyleUtils';
Expand All @@ -18,9 +16,14 @@ import colors from '../styles/colors';
import variables from '../styles/variables';
import MultipleAvatars from './MultipleAvatars';
import * as defaultWorkspaceAvatars from './Icon/WorkspaceDefaultAvatars';
import PressableWithSecondaryInteraction from './PressableWithSecondaryInteraction';
import withWindowDimensions, {windowDimensionsPropTypes} from './withWindowDimensions';
import * as DeviceCapabilities from '../libs/DeviceCapabilities';
import ControlSelection from '../libs/ControlSelection';

const propTypes = {
...menuItemPropTypes,
...windowDimensionsPropTypes,
};

const defaultProps = {
Expand All @@ -46,11 +49,13 @@ const defaultProps = {
subtitle: undefined,
iconType: CONST.ICON_TYPE_ICON,
onPress: () => {},
onSecondaryInteraction: undefined,
interactive: true,
fallbackIcon: Expensicons.FallbackAvatar,
brickRoadIndicator: '',
floatRightAvatars: [],
shouldStackHorizontally: false,
shouldBlockSelection: false,
};

const MenuItem = (props) => {
Expand All @@ -61,6 +66,7 @@ const MenuItem = (props) => {
(props.interactive && props.disabled ? {...styles.disabledText, ...styles.userSelectNone} : undefined),
styles.pre,
styles.ltr,
(_.contains(props.style, styles.offlineFeedback.deleted) ? styles.offlineFeedback.deleted : undefined),
], props.titleStyle);
const descriptionVerticalMargin = props.shouldShowDescriptionOnTop ? styles.mb1 : styles.mt1;
const descriptionTextStyle = StyleUtils.combineStyles([
Expand All @@ -72,7 +78,7 @@ const MenuItem = (props) => {
]);

return (
<Pressable
<PressableWithSecondaryInteraction
onPress={(e) => {
if (props.disabled) {
return;
Expand All @@ -84,12 +90,16 @@ const MenuItem = (props) => {

props.onPress(e);
}}
onPressIn={() => props.shouldBlockSelection && props.isSmallScreenWidth && DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}
onPressOut={ControlSelection.unblock}
onSecondaryInteraction={props.onSecondaryInteraction}
style={({hovered, pressed}) => ([
props.style,
StyleUtils.getButtonBackgroundColorStyle(getButtonState(props.focused || hovered, pressed, props.success, props.disabled, props.interactive), true),
..._.isArray(props.wrapperStyle) ? props.wrapperStyle : [props.wrapperStyle],
])}
disabled={props.disabled}
ref={props.forwardedRef}
>
{({hovered, pressed}) => (
<>
Expand Down Expand Up @@ -212,12 +222,18 @@ const MenuItem = (props) => {
</View>
</>
)}
</Pressable>
</PressableWithSecondaryInteraction>
);
};

MenuItem.propTypes = propTypes;
MenuItem.defaultProps = defaultProps;
MenuItem.displayName = 'MenuItem';

export default MenuItem;
const MenuItemWithWindowDimensions = withWindowDimensions(React.forwardRef((props, ref) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

That is a weird bug.
Thanks for coming up with a fix so fast, but I wonder why this is happening and how could we fix this without applying workarounds.
I'll try to investigate this further a bit before we can merge this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I have an additional fix incoming for this issue: #17656. Just need to verify the details first.

// eslint-disable-next-line react/jsx-props-no-spreading
<MenuItem {...props} forwardedRef={ref} />
)));
MenuItemWithWindowDimensions.defaultProps = defaultProps;

export default MenuItemWithWindowDimensions;
45 changes: 34 additions & 11 deletions src/components/MenuItemList.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import _ from 'underscore';
import PropTypes from 'prop-types';
import MenuItem from './MenuItem';
import menuItemPropTypes from './menuItemPropTypes';
import * as ReportActionContextMenu from '../pages/home/report/ContextMenu/ReportActionContextMenu';
import {CONTEXT_MENU_TYPES} from '../pages/home/report/ContextMenu/ContextMenuActions';

const propTypes = {
/** An array of props that are pass to individual MenuItem components */
Expand All @@ -12,17 +14,38 @@ const defaultProps = {
menuItems: [],
};

const MenuItemList = props => (
<>
{_.map(props.menuItems, menuItemProps => (
<MenuItem
key={menuItemProps.title}
// eslint-disable-next-line react/jsx-props-no-spreading
{...menuItemProps}
/>
))}
</>
);
const MenuItemList = (props) => {
let popoverAnchor;

/**
* Handle the secondary interaction for a menu item.
*
* @param {*} link the menu item link or function to get the link
* @param {Event} e the interaction event
*/
const secondaryInteraction = (link, e) => {
if (typeof link === 'function') {
link().then(url => ReportActionContextMenu.showContextMenu(CONTEXT_MENU_TYPES.LINK, e, url, popoverAnchor));
} else if (!_.isEmpty(link)) {
ReportActionContextMenu.showContextMenu(CONTEXT_MENU_TYPES.LINK, e, link, popoverAnchor);
}
};

return (
<>
{_.map(props.menuItems, menuItemProps => (
<MenuItem
key={menuItemProps.title}
onSecondaryInteraction={!_.isUndefined(menuItemProps.link) ? e => secondaryInteraction(menuItemProps.link, e) : undefined}
ref={el => popoverAnchor = el}
shouldBlockSelection={Boolean(menuItemProps.link)}
// eslint-disable-next-line react/jsx-props-no-spreading
{...menuItemProps}
/>
))}
</>
);
};

MenuItemList.displayName = 'MenuItemList';
MenuItemList.propTypes = propTypes;
Expand Down
31 changes: 21 additions & 10 deletions src/components/PressableWithSecondaryInteraction/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ import {Pressable} from 'react-native';
import * as pressableWithSecondaryInteractionPropTypes from './pressableWithSecondaryInteractionPropTypes';
import styles from '../../styles/styles';
import * as DeviceCapabilities from '../../libs/DeviceCapabilities';
import * as StyleUtils from '../../styles/StyleUtils';

/**
* This is a special Pressable that calls onSecondaryInteraction when LongPressed, or right-clicked.
*/
class PressableWithSecondaryInteraction extends Component {
constructor(props) {
super(props);
this.executeSecondaryInteraction = this.executeSecondaryInteraction.bind(this);
this.executeSecondaryInteractionOnContextMenu = this.executeSecondaryInteractionOnContextMenu.bind(this);
}

Expand All @@ -25,11 +27,28 @@ class PressableWithSecondaryInteraction extends Component {
this.pressableRef.removeEventListener('contextmenu', this.executeSecondaryInteractionOnContextMenu);
}

/**
* @param {Event} e - the secondary interaction event
*/
executeSecondaryInteraction(e) {
if (DeviceCapabilities.hasHoverSupport()) {
return;
}
if (this.props.withoutFocusOnSecondaryInteraction && this.pressableRef) {
this.pressableRef.blur();
}
this.props.onSecondaryInteraction(e);
}

/**
* @param {contextmenu} e - A right-click MouseEvent.
* https://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event
*/
executeSecondaryInteractionOnContextMenu(e) {
if (!this.props.onSecondaryInteraction) {
return;
}

e.stopPropagation();
if (this.props.preventDefaultContextMenu) {
e.preventDefault();
Expand All @@ -54,17 +73,9 @@ class PressableWithSecondaryInteraction extends Component {
// On Web, Text does not support LongPress events thus manage inline mode with styling instead of using Text.
return (
<Pressable
style={this.props.inline && styles.dInline}
style={StyleUtils.combineStyles(this.props.inline ? styles.dInline : this.props.style)}
onPressIn={this.props.onPressIn}
onLongPress={(e) => {
if (DeviceCapabilities.hasHoverSupport()) {
return;
}
if (this.props.withoutFocusOnSecondaryInteraction && this.pressableRef) {
this.pressableRef.blur();
}
this.props.onSecondaryInteraction(e);
}}
onLongPress={this.props.onSecondaryInteraction ? this.executeSecondaryInteraction : undefined}
onPressOut={this.props.onPressOut}
onPress={this.props.onPress}
ref={el => this.pressableRef = el}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ const PressableWithSecondaryInteraction = (props) => {
ref={props.forwardedRef}
onPress={props.onPress}
onLongPress={(e) => {
if (!props.onSecondaryInteraction) {
return;
}
e.preventDefault();
HapticFeedback.longPress();
props.onSecondaryInteraction(e);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import PropTypes from 'prop-types';
import stylePropTypes from '../../styles/stylePropTypes';

const propTypes = {
/** The function that should be called when this pressable is pressed */
Expand All @@ -11,13 +12,19 @@ const propTypes = {
onPressOut: PropTypes.func,

/** The function that should be called when this pressable is LongPressed or right-clicked. */
onSecondaryInteraction: PropTypes.func.isRequired,
onSecondaryInteraction: PropTypes.func,

/** The children which should be contained in this wrapper component. */
children: PropTypes.node.isRequired,
children: PropTypes.oneOfType([
PropTypes.func,
PropTypes.node,
]).isRequired,

/** The ref to the search input (may be null on small screen widths) */
forwardedRef: PropTypes.func,
forwardedRef: PropTypes.oneOfType([
PropTypes.func,
PropTypes.object,
]),

/** Prevent the default ContextMenu on web/Desktop */
preventDefaultContextMenu: PropTypes.bool,
Expand All @@ -34,6 +41,9 @@ const propTypes = {

/** Disable focus trap for the element on secondary interaction */
withoutFocusOnSecondaryInteraction: PropTypes.bool,

/** Used to apply styles to the Pressable */
style: stylePropTypes,
};

const defaultProps = {
Expand Down
12 changes: 12 additions & 0 deletions src/components/menuItemPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,18 @@ const propTypes = {

/** Prop to identify if we should load avatars vertically instead of diagonally */
shouldStackHorizontally: PropTypes.bool,

/** The function that should be called when this component is LongPressed or right-clicked. */
onSecondaryInteraction: PropTypes.func,

/** Flag to indicate whether or not text selection should be disabled from long-pressing the menu item. */
shouldBlockSelection: PropTypes.bool,

/** The ref to the menu item */
forwardedRef: PropTypes.oneOfType([
PropTypes.func,
PropTypes.object,
]),
};

export default propTypes;
47 changes: 25 additions & 22 deletions src/libs/actions/Link.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,33 +34,35 @@ function showGrowlIfOffline() {
}

/**
* @param {String} url
* @param {String} [url] the url path
* @param {String} [shortLivedAuthToken]
*
* @returns {Promise<string>}
*/
function openOldDotLink(url) {
/**
* @param {String} [shortLivedAuthToken]
* @returns {Promise<string>}
*/
function buildOldDotURL(shortLivedAuthToken) {
const hasHashParams = url.indexOf('#') !== -1;
const hasURLParams = url.indexOf('?') !== -1;
function buildOldDotURL(url, shortLivedAuthToken) {
const hasHashParams = url.indexOf('#') !== -1;
const hasURLParams = url.indexOf('?') !== -1;
Comment on lines -37 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

@akinwale @eVoloshchak can you remind me please why is this change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To summarise: Some of the menu items made use of the openOldDotLink(url) method, which built the URLs using the inner defined buildOldDotURL method. Since the original issue scope was to make the "Copy to clipboard" behaviour happen for all links, I had to refactor this method to the outer scope.

You can follow the discussion here for more details: #17310 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@akinwale Ok thank you, can you also add a Qa test which will check the deeplink works as expected, also tehre is a merge conflict, I will merge once this is resolved, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mountiny All done!


const authTokenParam = shortLivedAuthToken ? `authToken=${shortLivedAuthToken}` : '';
const emailParam = `email=${encodeURIComponent(currentUserEmail)}`;
const authTokenParam = shortLivedAuthToken ? `authToken=${shortLivedAuthToken}` : '';
const emailParam = `email=${encodeURIComponent(currentUserEmail)}`;

const params = _.compact([authTokenParam, emailParam]).join('&');
const params = _.compact([authTokenParam, emailParam]).join('&');

return Environment.getOldDotEnvironmentURL()
.then((environmentURL) => {
const oldDotDomain = Url.addTrailingForwardSlash(environmentURL);
return Environment.getOldDotEnvironmentURL()
.then((environmentURL) => {
const oldDotDomain = Url.addTrailingForwardSlash(environmentURL);

// If the URL contains # or ?, we can assume they don't need to have the `?` token to start listing url parameters.
return `${oldDotDomain}${url}${hasHashParams || hasURLParams ? '&' : '?'}${params}`;
});
}
// If the URL contains # or ?, we can assume they don't need to have the `?` token to start listing url parameters.
return `${oldDotDomain}${url}${hasHashParams || hasURLParams ? '&' : '?'}${params}`;
});
}

/**
* @param {String} url the url path
*/
function openOldDotLink(url) {
if (isNetworkOffline) {
buildOldDotURL().then(oldDotURL => Linking.openURL(oldDotURL));
buildOldDotURL(url).then(oldDotURL => Linking.openURL(oldDotURL));
return;
}

Expand All @@ -69,11 +71,11 @@ function openOldDotLink(url) {
API.makeRequestWithSideEffects(
'OpenOldDotLink', {}, {},
).then((response) => {
buildOldDotURL(response.shortLivedAuthToken).then((oldDotUrl) => {
buildOldDotURL(url, response.shortLivedAuthToken).then((oldDotUrl) => {
Linking.openURL(oldDotUrl);
});
}).catch(() => {
buildOldDotURL().then((oldDotUrl) => {
buildOldDotURL(url).then((oldDotUrl) => {
Linking.openURL(oldDotUrl);
});
});
Expand All @@ -92,6 +94,7 @@ function openExternalLink(url, shouldSkipCustomSafariLogic = false) {
}

export {
buildOldDotURL,
openOldDotLink,
openExternalLink,
};
1 change: 1 addition & 0 deletions src/pages/GetAssistancePage.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ const GetAssistancePage = (props) => {
shouldShowRightIcon: true,
iconRight: Expensicons.NewWindow,
wrapperStyle: [styles.cardMenuItem],
link: CONST.NEWHELP_URL,
}];

// If the user is eligible for calls with their Guide, add the 'Schedule a setup call' item at the second position in the list
Expand Down
Loading