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
125 changes: 125 additions & 0 deletions src/components/OfflineWithFeedback.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
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';

/**
* 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 isOfflinePendingAction = props.network.isOffline && props.pendingAction;
const isUpdateOrDeleteError = props.errors && (props.pendingAction === 'delete' || props.pendingAction === 'update');
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: I think you are considering props.errors equal null when there are no errors. Just wondering if this should also consider {} as no errors

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this should be left as it is and we should not expect {}... i don't like having two different things representing the same (no errors)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good question... I think you are right and I should be using _.isEmpty. Changing it.

Copy link
Contributor Author

@iwiznia iwiznia Jul 22, 2022

Choose a reason for hiding this comment

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

To clarify, when we have fieldErrors (or errorFields don't recall what we settled in), then it's totally possible that this would end up being an empty object (since onyx can't really clear things)

const isAddError = props.errors && props.pendingAction === 'add';
const needsOpacity = (isOfflinePendingAction && !isUpdateOrDeleteError) || isAddError;
const needsStrikeThrough = props.network.isOffline && props.pendingAction === 'delete';
const hideChildren = !props.network.isOffline && props.pendingAction === 'delete' && !props.errors;
let children = props.children;
const sortedErrors = _.map(_.sortBy(_.keys(props.errors)), key => props.errors[key]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const sortedErrors = _.map(_.sortBy(_.keys(props.errors)), key => props.errors[key]);
const sortedErrors = _.chain(props.errors)
.keys()
.sortBy()
.map(key => props.errors[key])
.value();

This may be more readable


// 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>
)}
{props.errors && (
<View style={styles.offlineFeedback.error}>
<View style={styles.offlineFeedback.errorDot}>
<Icon src={Expensicons.DotIndicator} fill={colors.red} height={16} width={16} />
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: replace 16 by variables.iconSizeSmall (import variables from '../styles/variables';)

</View>
<View style={{flexDirection: 'column', alignItems: 'center', flex: 1}}>
{_.map(sortedErrors, error => (
<Text 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,
};
29 changes: 29 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,32 @@ const styles = {
...whiteSpace.noWrap,
},

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

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