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

17047 migrate PressableWithSecondaryInteraction to PressableWithFeedback component #20251

Merged
Show file tree
Hide file tree
Changes from 6 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
7 changes: 7 additions & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -2448,6 +2448,13 @@ const CONST = {
EXPENSIFY_LOGO_SIZE_RATIO: 0.22,
EXPENSIFY_LOGO_MARGIN_RATIO: 0.03,
},
ACCESSIBILITY_ROLE: {
BUTTON: 'button',
LINK: 'link',
MENUITEM: 'menuitem',
TEXT: 'text',
RADIO: 'radio',
},
};

export default CONST;
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import * as DeviceCapabilities from '../../libs/DeviceCapabilities';
import styles from '../../styles/styles';
import withWindowDimensions, {windowDimensionsPropTypes} from '../withWindowDimensions';
import {propTypes as anchorForCommentsOnlyPropTypes, defaultProps as anchorForCommentsOnlyDefaultProps} from './anchorForCommentsOnlyPropTypes';
import CONST from '../../CONST';

const propTypes = {
/** Press in handler for the link */
Expand Down Expand Up @@ -65,6 +66,8 @@ const BaseAnchorForCommentsOnly = (props) => {
onPress={linkProps.onPress}
onPressIn={props.onPressIn}
onPressOut={props.onPressOut}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.LINK}
accessibilityLabel={props.href}
>
<Tooltip text={props.href}>
<Text
Expand Down
4 changes: 3 additions & 1 deletion src/components/LHNOptionsList/OptionRowLHN.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,12 @@ const OptionRowLHN = (props) => {
props.isFocused ? styles.sidebarLinkActive : null,
hovered && !props.isFocused ? props.hoverStyle : null,
]}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
accessibilityLabel={props.translate('accessibilityHints.navigatesToChat')}
>
<View
accessibilityHint={props.translate('accessibilityHints.navigatesToChat')}
style={sidebarInnerRowStyle}
accessibilityHint={props.translate('accessibilityHints.navigatesToChat')}
>
<View style={[styles.flexRow, styles.alignItemsCenter]}>
{!_.isEmpty(optionItem.icons) &&
Expand Down
2 changes: 2 additions & 0 deletions src/components/MenuItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ const MenuItem = (props) => {
]}
disabled={props.disabled}
ref={props.forwardedRef}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.MENUITEM}
accessibilityLabel={props.title}
Copy link
Contributor

@abdulrahuman5196 abdulrahuman5196 Sep 4, 2023

Choose a reason for hiding this comment

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

props.title was of number 0 in some cases which caused app crash as mentioned here #25451 (comment).
We should make sure the values we pass are string.

>
{({hovered, pressed}) => (
<>
Expand Down
8 changes: 4 additions & 4 deletions src/components/PressableWithSecondaryInteraction/index.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import _ from 'underscore';
import React, {Component} from 'react';
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';
import PressableWithFeedback from '../Pressable/PressableWithFeedback';

/**
* This is a special Pressable that calls onSecondaryInteraction when LongPressed, or right-clicked.
Expand Down Expand Up @@ -76,19 +76,19 @@ class PressableWithSecondaryInteraction extends Component {

// On Web, Text does not support LongPress events thus manage inline mode with styling instead of using Text.
return (
<Pressable
<PressableWithFeedback
style={StyleUtils.combineStyles(this.props.inline ? styles.dInline : this.props.style)}
onPressIn={this.props.onPressIn}
onLongPress={this.props.onSecondaryInteraction ? this.executeSecondaryInteraction : undefined}
activeOpacity={this.props.activeOpacity}
pressDimmingValue={this.props.activeOpacity}
onPressOut={this.props.onPressOut}
onPress={this.props.onPress}
ref={(el) => (this.pressableRef = el)}
// eslint-disable-next-line react/jsx-props-no-spreading
{...defaultPressableProps}
>
{this.props.children}
</Pressable>
</PressableWithFeedback>
);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import _ from 'underscore';
import React, {forwardRef} from 'react';
import {Pressable} from 'react-native';
import * as pressableWithSecondaryInteractionPropTypes from './pressableWithSecondaryInteractionPropTypes';
import Text from '../Text';
import HapticFeedback from '../../libs/HapticFeedback';
import PressableWithFeedback from '../Pressable/PressableWithFeedback';

/**
* This is a special Pressable that calls onSecondaryInteraction when LongPressed.
Expand All @@ -13,22 +12,20 @@ import HapticFeedback from '../../libs/HapticFeedback';
*/
const PressableWithSecondaryInteraction = (props) => {
// Use Text node for inline mode to prevent content overflow.
const Node = props.inline ? Text : Pressable;
const Node = props.inline ? Text : PressableWithFeedback;
const executeSecondaryInteraction = (e) => {
e.preventDefault();
props.onSecondaryInteraction(e);
};

return (
<Node
ref={props.forwardedRef}
onPress={props.onPress}
onLongPress={(e) => {
if (!props.onSecondaryInteraction) {
return;
}
e.preventDefault();
HapticFeedback.longPress();
props.onSecondaryInteraction(e);
}}
onLongPress={props.onSecondaryInteraction ? executeSecondaryInteraction : undefined}
onPressIn={props.onPressIn}
onPressOut={props.onPressOut}
activeOpacity={props.activeOpacity}
pressDimmingValue={props.activeOpacity}
// eslint-disable-next-line react/jsx-props-no-spreading
{..._.omit(props, 'onLongPress')}
>
Expand Down
2 changes: 2 additions & 0 deletions src/components/Reactions/EmojiReactionBubble.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ const EmojiReactionBubble = (props) => (
enableLongPressWithHover={props.isSmallScreenWidth}
// Prevent text input blur when emoji reaction is clicked
onMouseDown={(e) => e.preventDefault()}
accessibilityRole="button"
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB since this already exists in many places.

Suggested change
accessibilityRole="button"
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}

@luacmartins after this PR merged, let's create follow-up issue for replacing all hardcoded roles

accessibilityLabel={props.emojiCodes.join('')}
>
<Text style={[styles.emojiReactionBubbleText, styles.userSelectNone, StyleUtils.getEmojiReactionBubbleTextStyle(props.isContextMenu)]}>{props.emojiCodes.join('')}</Text>
{props.count > 0 && <Text style={[styles.reactionCounterText, styles.userSelectNone, StyleUtils.getEmojiReactionCounterTextStyle(props.hasUserReacted)]}>{props.count}</Text>}
Expand Down