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

Add OfflineWithFeedback component #9931

Merged
merged 15 commits into from
Jul 22, 2022
Merged
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ You can use any IDE or code editing tool for developing on any platform. Use you
For an M1 Mac, read this [SO](https://stackoverflow.com/c/expensify/questions/11580) for installing cocoapods.

* To install the iOS dependencies, run: `npm install && cd ios/ && pod install && cd ..`
* If you are an Expensify employee and want to point the emulator to your local VM, follow [this](https://stackoverflow.com/c/expensify/questions/7699)
* To run a on a **Development Simulator**: `npm run ios`
* Changes applied to Javascript will be applied automatically, any changes to native code will require a recompile

Expand All @@ -53,6 +54,7 @@ For an M1 Mac, read this [SO](https://stackoverflow.com/c/expensify/questions/11
* To install the Android dependencies, run: `npm install`
* Go through the instructions on [this SO post](https://stackoverflow.com/c/expensify/questions/13283/13284#13284) to start running the app on android.
* For more information, go through the official React-Native instructions on [this page](https://reactnative.dev/docs/environment-setup#development-os) for "React Native CLI Quickstart" > Mac OS > Android
* If you are an Expensify employee and want to point the emulator to your local VM, follow [this](https://stackoverflow.com/c/expensify/questions/7699)
* To run a on a **Development Emulator**: `npm run android`
* Changes applied to Javascript will be applied automatically, any changes to native code will require a recompile

Expand Down
6 changes: 6 additions & 0 deletions assets/images/dot-indicator.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 2 additions & 0 deletions src/components/Icon/Expensicons.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import ClosedSign from '../../../assets/images/closed-sign.svg';
import Collapse from '../../../assets/images/collapse.svg';
import Concierge from '../../../assets/images/concierge.svg';
import CreditCard from '../../../assets/images/creditcard.svg';
import DotIndicator from '../../../assets/images/dot-indicator.svg';
import DownArrow from '../../../assets/images/down.svg';
import Download from '../../../assets/images/download.svg';
import Emoji from '../../../assets/images/emoji.svg';
Expand Down Expand Up @@ -109,6 +110,7 @@ export {
CreditCard,
DeletedRoomAvatar,
DomainRoomAvatar,
DotIndicator,
DownArrow,
Download,
Emoji,
Expand Down
131 changes: 131 additions & 0 deletions src/components/OfflineWithFeedback.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import _ from 'underscore';
import React from 'react';
import {Pressable, View} from 'react-native';
import PropTypes from 'prop-types';
import compose from '../libs/compose';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import {withNetwork} from './OnyxProvider';
import networkPropTypes from './networkPropTypes';
import Text from './Text';
import styles from '../styles/styles';
import Tooltip from './Tooltip';
import Icon from './Icon';
import * as Expensicons from './Icon/Expensicons';
import * as StyleUtils from '../styles/StyleUtils';
import colors from '../styles/colors';
import variables from '../styles/variables';

/**
* This component should be used when we are using the offline pattern B (offline with feedback).
* You should enclose any element that should have feedback that the action was taken offline and it will take
* care of adding the appropriate styles for pending actions and displaying the dismissible error.
*/

const propTypes = {
/** The type of action that's pending */
pendingAction: PropTypes.oneOf(['add', 'update', 'delete']),

/** The errors to display */
// eslint-disable-next-line react/forbid-prop-types
errors: PropTypes.object,

/** A function to run when the X button next to the error is clicked */
onClose: PropTypes.func.isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when I don't want anything to happen when I click the close button, do I need to strictly pass a method that does nothing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that you always want to do something with the X button. It would be weird to have an X button that does nothing :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heh yes, I think you will always have to pass one, though now I realize that maybe we should have this component remove the error box by default and in that case maybe there are actions that do not require an onClose.... though not sure, I will leave this as required, we can remove the required later when we find a case that does not need it.


/** The content that needs offline feedback */
children: PropTypes.node.isRequired,

/** Information about the network */
network: networkPropTypes.isRequired,

/** Additional styles to add after local styles. Applied to Pressable portion of button */
style: PropTypes.oneOfType([
PropTypes.arrayOf(PropTypes.object),
PropTypes.object,
]),
...withLocalizePropTypes,
};

const defaultProps = {
pendingAction: null,
errors: null,
style: [],
};

/**
* This method applies the strikethrough to all the children passed recursively
* @param {Array} children
* @return {Array}
*/
function applyStrikeThrough(children) {

This comment was marked as resolved.

return React.Children.map(children, (child) => {
if (!React.isValidElement(child)) {
return child;
}
const props = {style: StyleUtils.combineStyles(child.props.style, styles.offlineFeedback.deleted)};
if (child.props.children) {
props.children = applyStrikeThrough(child.props.children);
}
return React.cloneElement(child, props);
});
}

const OfflineWithFeedback = (props) => {
const hasErrors = !_.isEmpty(props.errors);
const isOfflinePendingAction = props.network.isOffline && props.pendingAction;
const isUpdateOrDeleteError = hasErrors && (props.pendingAction === 'delete' || props.pendingAction === 'update');
const isAddError = hasErrors && props.pendingAction === 'add';
const needsOpacity = (isOfflinePendingAction && !isUpdateOrDeleteError) || isAddError;
const needsStrikeThrough = props.network.isOffline && props.pendingAction === 'delete';
const hideChildren = !props.network.isOffline && props.pendingAction === 'delete' && !hasErrors;
let children = props.children;
const sortedErrors = _.chain(props.errors)
.keys()
.sortBy()
.map(key => props.errors[key])
.value();

// Apply strikethrough to children if needed, but skip it if we are not going to render them
if (needsStrikeThrough && !hideChildren) {
children = applyStrikeThrough(children);
}
return (
<View style={props.style}>
{!hideChildren && (
<View style={needsOpacity ? styles.offlineFeedback.pending : {}}>
{children}
</View>
)}
{hasErrors && (
<View style={styles.offlineFeedback.error}>
<View style={styles.offlineFeedback.errorDot}>
<Icon src={Expensicons.DotIndicator} fill={colors.red} height={variables.iconSizeSmall} width={variables.iconSizeSmall} />
</View>
<View style={styles.offlineFeedback.textContainer}>
{_.map(sortedErrors, (error, i) => (
Copy link
Contributor

@aldo-expensify aldo-expensify Jul 22, 2022

Choose a reason for hiding this comment

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

NAB: because it is hard for me to tell if this has a real impact or not, but it is not recommended by the react doc.

It is not recommended to use indexes (i) as a key, from react doc:

We don’t recommend using indexes for keys if the order of items may change. This can negatively impact performance and may cause issues with component state. Check out Robin Pokorny’s article for an in-depth explanation on the negative impacts of using an index as a key. If you choose not to assign an explicit key to list items then React will default to using indexes as keys.

Not sure if it will have a real impact here... but an easy option to it would have been:

const sortedErrorKeys = _.chain(props.errors)
        .keys()
        .sortBy()
       // .map(key => props.errors[key]) Don't map this to values!
        .value();

then, in your map here you would use the error's key as the key:

{_.map(sortedErrorKeys, (errorKey) => (
    <Text key={errorKey} style={styles.offlineFeedback.text}>{props.errors[errorKey]}</Text>
))}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don’t recommend using indexes for keys if the order of items may change

The order of items will never change, so I think we are fine here, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm the index for one of the errors may change if one of the errors is deleted

<Text key={i} style={styles.offlineFeedback.text}>{error}</Text>
))}
</View>
<Tooltip text={props.translate('common.close')}>
<Pressable
onPress={props.onClose}
style={[styles.touchableButtonImage, styles.mr0]}
accessibilityRole="button"
accessibilityLabel={props.translate('common.close')}
>
<Icon src={Expensicons.Close} />
</Pressable>
</Tooltip>
</View>
)}
</View>
);
};

OfflineWithFeedback.propTypes = propTypes;
OfflineWithFeedback.defaultProps = defaultProps;

export default compose(
withLocalize,
withNetwork(),
)(OfflineWithFeedback);
11 changes: 7 additions & 4 deletions src/components/OptionRow.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ const propTypes = {
/** Whether this option should be disabled */
isDisabled: PropTypes.bool,

style: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]),

...withLocalizePropTypes,
};

Expand All @@ -77,6 +79,7 @@ const defaultProps = {
onSelectRow: () => {},
isDisabled: false,
optionIsFocused: false,
style: null,
};

const OptionRow = (props) => {
Expand All @@ -86,12 +89,12 @@ const OptionRow = (props) => {
: styles.sidebarLinkText;
const textUnreadStyle = (props.option.isUnread || props.forceTextUnreadStyle)
? [textStyle, styles.sidebarLinkTextUnread] : [textStyle];
const displayNameStyle = props.mode === CONST.OPTION_MODE.COMPACT
const displayNameStyle = StyleUtils.combineStyles(props.mode === CONST.OPTION_MODE.COMPACT
? [styles.optionDisplayName, ...textUnreadStyle, styles.optionDisplayNameCompact, styles.mr2]
: [styles.optionDisplayName, ...textUnreadStyle];
const alternateTextStyle = props.mode === CONST.OPTION_MODE.COMPACT
: [styles.optionDisplayName, ...textUnreadStyle], props.style);
const alternateTextStyle = StyleUtils.combineStyles(props.mode === CONST.OPTION_MODE.COMPACT
? [textStyle, styles.optionAlternateText, styles.textLabelSupporting, styles.optionAlternateTextCompact]
: [textStyle, styles.optionAlternateText, styles.textLabelSupporting];
: [textStyle, styles.optionAlternateText, styles.textLabelSupporting], props.style);
const contentContainerStyles = props.mode === CONST.OPTION_MODE.COMPACT
? [styles.flex1, styles.flexRow, styles.overflowHidden, styles.alignItemsCenter]
: [styles.flex1];
Expand Down
14 changes: 14 additions & 0 deletions src/styles/StyleUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,19 @@ function parseStyleAsArray(styleParam) {
return _.isArray(styleParam) ? styleParam : [styleParam];
}

/**
* Receives any number of object or array style objects and returns them all as an array
* @param {Object|Object[]} allStyles
* @return {Object[]}
*/
function combineStyles(...allStyles) {
let finalStyles = [];
_.each(allStyles, (style) => {
finalStyles = finalStyles.concat(parseStyleAsArray(style));
});
return finalStyles;
}

/**
* Get variable padding-left as style
* @param {Number} paddingLeft
Expand Down Expand Up @@ -474,5 +487,6 @@ export {
getMiniReportActionContextMenuWrapperStyle,
getPaymentMethodMenuWidth,
parseStyleAsArray,
combineStyles,
getPaddingLeft,
};
33 changes: 33 additions & 0 deletions src/styles/styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -2338,6 +2338,9 @@ const styles = {
width: '100%',
flexDirection: 'row',
justifyContent: 'space-between',
},

peopleRowOfflineFeedback: {
borderBottomWidth: 1,
borderColor: themeColors.border,
...spacing.pv2,
Expand All @@ -2359,6 +2362,36 @@ const styles = {
...whiteSpace.noWrap,
},

offlineFeedback: {
deleted: {
textDecorationLine: 'line-through',
textDecorationStyle: 'solid',
},
pending: {
opacity: 0.5,
},
error: {
flexDirection: 'row',
alignItems: 'center',
},
container: {
...spacing.pv2,
},
textContainer: {
flexDirection: 'column',
flex: 1,
},
text: {
color: themeColors.textSupporting,
flex: 1,
textAlignVertical: 'center',
fontSize: variables.fontSizeLabel,
},
errorDot: {
marginRight: 12,
},
},

sidebarPopover: {
width: variables.sideBarWidth - 68,
},
Expand Down