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

Added tapable links to user's login information on DetailsPage #3870

Merged
merged 11 commits into from
Jul 12, 2021
79 changes: 79 additions & 0 deletions src/components/CommunicationsLink.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import React from 'react';
import {View, Pressable, Linking} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../styles/styles';
import compose from '../libs/compose';
import {Checkmark, Clipboard as ClipboardIcon} from './Icon/Expensicons';
import Clipboard from '../libs/Clipboard';
import ContextMenuItem from './ContextMenuItem';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import withWindowDimensions, {windowDimensionsPropTypes} from './withWindowDimensions';
import CONST from '../CONST';

const propTypes = {
/** Children to wrap in CommunicationsLink. */
children: PropTypes.node.isRequired,

/** Styles to be assigned to Container */
containerStyles: PropTypes.arrayOf(PropTypes.object),

/** Decides Tap behaviour. */
type: PropTypes.oneOf([CONST.LOGIN_TYPE.PHONE, CONST.LOGIN_TYPE.EMAIL]),

/** Value to be copied or passed via tap. */
value: PropTypes.string.isRequired,

...windowDimensionsPropTypes,
...withLocalizePropTypes,
};

const defaultProps = {
containerStyles: [],
type: undefined,
};

const CommunicationsLink = props => (
<View style={[styles.flexRow, styles.pRelative, ...props.containerStyles]}>
{props.type && props.isSmallScreenWidth
? (
<Pressable
onPress={() => Linking.openURL(
props.type === CONST.LOGIN_TYPE.PHONE
? `tel:${props.value}`
: `mailto:${props.value}`,
)}
>
{props.children}
</Pressable>
)
: props.children}
{props.type && !props.isSmallScreenWidth
&& (
<View style={[
styles.pAbsolute,
styles.alignItemsCenter,
styles.justifyContentCenter,
styles.communicationsLinkIcon]}
>
<ContextMenuItem
icon={ClipboardIcon}
text={props.translate('contextMenuItem.copyToClipboard')}
successIcon={Checkmark}
successText={props.translate('contextMenuItem.copied')}
isMini
autoReset
onPress={() => Clipboard.setString(props.value)}
/>
</View>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Still don't get the checks for props.type. What is using this without a type? If there's no type we show props.children? We should not use it at all in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

To make CommunicationLink uses cleaner I did this. Otherwise, we have to conditionally show the content wrapped in CommunicationLink which will duplicate a few lines on the main Component and This approach gives it a cleaner usage. https://github.com/parasharrajat/Expensify.cash/blob/a6814422a60fbba13d04e19f2807190a82450c2f/src/pages/DetailsPage.js#L95.

I will welcome suggestions to improve it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I am removing the confusing uses of Type. Instead, just use the conditional block here https://github.com/parasharrajat/Expensify.cash/blob/a6814422a60fbba13d04e19f2807190a82450c2f/src/pages/DetailsPage.js#L95

Copy link
Contributor

@marcaaron marcaaron Jul 12, 2021

Choose a reason for hiding this comment

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

Perfect thanks!

Otherwise, we have to conditionally show the content wrapped in CommunicationLink which will duplicate a few lines on the main Component and This approach gives it a cleaner usage.

I have a different opinion. To me it's not clear why we should use a component that might do nothing at all. It's as if we are calling a function with an optional argument that specifies whether we should call the function or not. In this case, it's clearer to just not call the function in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for clarifying.

</View>
);

CommunicationsLink.propTypes = propTypes;
CommunicationsLink.defaultProps = defaultProps;
CommunicationsLink.displayName = 'CommunicationsLink';

export default compose(
withWindowDimensions,
withLocalize,
)(CommunicationsLink);
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import React, {Component} from 'react';
import PropTypes from 'prop-types';
import {Pressable} from 'react-native';
import MenuItem from '../../../components/MenuItem';
import Tooltip from '../../../components/Tooltip';
import Icon from '../../../components/Icon';
import styles, {getIconFillColor, getButtonBackgroundColorStyle} from '../../../styles/styles';
import getButtonState from '../../../libs/getButtonState';
import MenuItem from './MenuItem';
import Tooltip from './Tooltip';
import Icon from './Icon';
import styles, {getIconFillColor, getButtonBackgroundColorStyle} from '../styles/styles';
import getButtonState from '../libs/getButtonState';

const propTypes = {
/** Icon Component */
Expand All @@ -25,15 +25,19 @@ const propTypes = {

/** Callback to fire when the item is pressed */
onPress: PropTypes.func.isRequired,

/** Automatically reset the success status */
autoReset: PropTypes.bool,
};

const defaultProps = {
isMini: false,
successIcon: null,
successText: '',
autoReset: false,
};

class ReportActionContextMenuItem extends Component {
class ContextMenuItem extends Component {
constructor(props) {
super(props);
this.state = {
Expand All @@ -42,6 +46,12 @@ class ReportActionContextMenuItem extends Component {
this.triggerPressAndUpdateSuccess = this.triggerPressAndUpdateSuccess.bind(this);
}

componentWillUnmount() {
if (this.successResetTimer) {
clearTimeout(this.successResetTimer);
}
}

/**
* Called on button press and mark the run
*/
Expand All @@ -57,6 +67,9 @@ class ReportActionContextMenuItem extends Component {
this.setState({
success: true,
});
if (this.props.autoReset) {
this.successResetTimer = setTimeout(() => this.setState({success: false}), 1800);
}
}
}

Expand Down Expand Up @@ -99,8 +112,8 @@ class ReportActionContextMenuItem extends Component {
}
}

ReportActionContextMenuItem.propTypes = propTypes;
ReportActionContextMenuItem.defaultProps = defaultProps;
ReportActionContextMenuItem.displayName = 'ReportActionContextMenuItem';
ContextMenuItem.propTypes = propTypes;
ContextMenuItem.defaultProps = defaultProps;
ContextMenuItem.displayName = 'ContextMenuItem';

export default ReportActionContextMenuItem;
export default ContextMenuItem;
4 changes: 3 additions & 1 deletion src/languages/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,11 @@ export default {
fileUploadFailed: 'Upload Failed. File is not supported.',
localTime: ({user, time}) => `It's ${time} for ${user}`,
},
reportActionContextMenu: {
contextMenuItem: {
copyToClipboard: 'Copy to Clipboard',
copied: 'Copied!',
},
reportActionContextMenu: {
copyLink: 'Copy Link',
markAsUnread: 'Mark as Unread',
editComment: 'Edit Comment',
Expand Down
38 changes: 25 additions & 13 deletions src/pages/DetailsPage.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import React from 'react';
import {
View,
} from 'react-native';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import Str from 'expensify-common/lib/str';
Expand All @@ -16,6 +14,8 @@ import ScreenWrapper from '../components/ScreenWrapper';
import personalDetailsPropType from './personalDetailsPropType';
import withLocalize, {withLocalizePropTypes} from '../components/withLocalize';
import compose from '../libs/compose';
import CommunicationsLink from '../components/CommunicationsLink';
import CONST from '../CONST';

const matchType = PropTypes.shape({
params: PropTypes.shape({
Expand Down Expand Up @@ -69,6 +69,7 @@ const DetailsPage = ({
const timezone = moment().tz(details.timezone.selected);
const GMTTime = `${timezone.toString().split(/[+-]/)[0].slice(-3)} ${timezone.zoneAbbr()}`;
const currentTime = Number.isNaN(Number(timezone.zoneAbbr())) ? timezone.zoneAbbr() : GMTTime;

return (
<ScreenWrapper>
<HeaderWithCloseButton
Expand All @@ -91,23 +92,34 @@ const DetailsPage = ({
imageStyles={[styles.avatarLarge]}
source={details.avatar}
/>
<Text style={[styles.displayName, styles.mt1, styles.mb6]} numberOfLines={1}>
{details.displayName && isSMSLogin
? toLocalPhone(details.displayName)
: (details.displayName || null)}
</Text>
<CommunicationsLink
style={[styles.mt1, styles.mb6]}
type={details.displayName && isSMSLogin ? CONST.LOGIN_TYPE.PHONE : undefined}
value={getPhoneNumber(details)}
>
<Text style={[styles.displayName]} numberOfLines={1}>
{details.displayName && isSMSLogin
? toLocalPhone(details.displayName)
: (details.displayName || null)}
</Text>
</CommunicationsLink>
{details.login ? (
<View style={[styles.mb6, styles.detailsPageSectionContainer]}>
<Text style={[styles.formLabel, styles.mb2]} numberOfLines={1}>
{translate(isSMSLogin
? 'common.phoneNumber'
: 'common.email')}
</Text>
<Text numberOfLines={1}>
{isSMSLogin
? toLocalPhone(getPhoneNumber(details))
: details.login}
</Text>
<CommunicationsLink
type={isSMSLogin ? CONST.LOGIN_TYPE.PHONE : CONST.LOGIN_TYPE.EMAIL}
value={isSMSLogin ? getPhoneNumber(details) : details.login}
>
<Text numberOfLines={1}>
{isSMSLogin
? toLocalPhone(getPhoneNumber(details))
: details.login}
</Text>
</CommunicationsLink>
</View>
) : null}
{details.pronouns ? (
Expand Down
10 changes: 5 additions & 5 deletions src/pages/home/report/ReportActionContextMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import getReportActionContextMenuStyles from '../../../styles/getReportActionCon
import {
setNewMarkerPosition, updateLastReadActionID, saveReportActionDraft,
} from '../../../libs/actions/Report';
import ReportActionContextMenuItem from './ReportActionContextMenuItem';
import ContextMenuItem from '../../../components/ContextMenuItem';
import ReportActionPropTypes from './ReportActionPropTypes';
import Clipboard from '../../../libs/Clipboard';
import compose from '../../../libs/compose';
Expand Down Expand Up @@ -67,14 +67,14 @@ class ReportActionContextMenu extends React.Component {
this.contextActions = [
// Copy to clipboard
{
text: this.props.translate('reportActionContextMenu.copyToClipboard'),
text: this.props.translate('contextMenuItem.copyToClipboard'),
icon: ClipboardIcon,
successText: this.props.translate('reportActionContextMenu.copied'),
successText: this.props.translate('contextMenuItem.copied'),
successIcon: Checkmark,
shouldShow: true,

// If return value is true, we switch the `text` and `icon` on
// `ReportActionContextMenuItem` with `successText` and `successIcon` which will fallback to
// `ContextMenuItem` with `successText` and `successIcon` which will fallback to
// the `text` and `icon`
onPress: () => {
const message = _.last(lodashGet(this.props.reportAction, 'message', null));
Expand Down Expand Up @@ -179,7 +179,7 @@ class ReportActionContextMenu extends React.Component {
return this.props.isVisible && (
<View style={this.wrapperStyle}>
{this.contextActions.map(contextAction => _.result(contextAction, 'shouldShow', false) && (
<ReportActionContextMenuItem
<ContextMenuItem
icon={contextAction.icon}
text={contextAction.text}
successIcon={contextAction.successIcon}
Expand Down
6 changes: 6 additions & 0 deletions src/styles/styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -1743,6 +1743,12 @@ const styles = {
lineHeight: 16,
...whiteSpace.noWrap,
},

communicationsLinkIcon: {
right: -36,
top: 0,
bottom: 0,
},
};

const baseCodeTagStyles = {
Expand Down