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

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

/** The error to display */
error: PropTypes.string,

/** 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,
error: null,
style: [],
};

class OfflineWithFeedback extends React.Component {
constructor(props) {
super(props);

this.state = {
};
aldo-expensify marked this conversation as resolved.
Show resolved Hide resolved
}

applyStrikeThrough(children) {
return React.Children.map(children, (child) => {
if (!React.isValidElement(child)) {
return child;
}

if (child.props.children) {
return React.cloneElement(child, {
style: combineStyles(child.props.style, styles.offlineFeedback.deleted),
children: this.applyStrikeThrough(child.props.children),
});
}

return React.cloneElement(child, {
style: combineStyles(child.props.style, styles.offlineFeedback.deleted),
});
});
}

render() {
const isOfflinePendingAction = this.props.network.isOffline && this.props.pendingAction;
const isUpdateOrDeleteError = this.props.error && (this.props.pendingAction === 'delete' || this.props.pendingAction === 'update');
const isAddError = this.props.error && this.props.pendingAction === 'add';
const needsOpacity = (isOfflinePendingAction && !isUpdateOrDeleteError) || isAddError;
const needsStrikeThrough = this.props.network.isOffline && this.props.pendingAction === 'delete';
const hideChildren = !this.props.network.isOffline && this.props.pendingAction === 'delete';
aldo-expensify marked this conversation as resolved.
Show resolved Hide resolved
let children = this.props.children;

// Apply strikethrough to children if needed, but skip it if we are not going to render them
if (needsStrikeThrough && !hideChildren) {
children = this.applyStrikeThrough(children);
}
return (
<View style={this.props.style}>
{!hideChildren && (
<View style={needsOpacity ? styles.offlineFeedback.pending : {}}>
{children}
</View>
)}
{this.props.error && (
<View style={styles.offlineFeedback.error}>
<View style={styles.offlineFeedback.errorDot} />
<Text style={styles.offlineFeedback.text}>{this.props.error}</Text>
<Tooltip text={this.props.translate('common.close')} containerStyles={styles.offlineFeedback.close}>
<Pressable
onPress={this.props.onClose}
style={[styles.touchableButtonImage, styles.mr0]}
accessibilityRole="button"
accessibilityLabel={this.props.translate('common.close')}
>
<Icon src={Expensicons.Close} />
</Pressable>
</Tooltip>
</View>
)}
</View>
);
}
}

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

export default compose(
withLocalize,
withNetwork(),
)(OfflineWithFeedback);
63 changes: 33 additions & 30 deletions src/pages/workspace/WorkspaceMembersPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import CheckboxWithTooltip from '../../components/CheckboxWithTooltip';
import Hoverable from '../../components/Hoverable';
import withFullPolicy, {fullPolicyPropTypes, fullPolicyDefaultProps} from './withFullPolicy';
import CONST from '../../CONST';
import OfflineWithFeedback from '../../components/OfflineWithFeedback';

const propTypes = {
/** The personal details of the person who is logged in */
Expand Down Expand Up @@ -203,45 +204,47 @@ class WorkspaceMembersPage extends React.Component {
}) {
const canBeRemoved = this.props.policy.owner !== item.login && this.props.session.email !== item.login;
return (
<Hoverable onHoverIn={() => this.willTooltipShowForLogin(item.login, true)} onHoverOut={() => this.setState({showTooltipForLogin: ''})}>
<TouchableOpacity
style={[styles.peopleRow, !canBeRemoved && styles.cursorDisabled]}
onPress={() => this.toggleUser(item.login)}
activeOpacity={0.7}
>
<CheckboxWithTooltip
style={[styles.peopleRowCell]}
isChecked={_.contains(this.state.selectedEmployees, item.login)}
disabled={!canBeRemoved}
<OfflineWithFeedback error="This is an errors This is an errors This is an errors This is an errors This is an errors This is an errors " style={styles.peopleRowOfflineFeedback} pendingAction="delete">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just for testing, I will either remove this from this PR or properly connect it to the Onyx data

<Hoverable onHoverIn={() => this.willTooltipShowForLogin(item.login, true)} onHoverOut={() => this.setState({showTooltipForLogin: ''})}>
<TouchableOpacity
style={[styles.peopleRow, !canBeRemoved && styles.cursorDisabled]}
onPress={() => this.toggleUser(item.login)}
toggleTooltip={this.state.showTooltipForLogin === item.login}
text={this.props.translate('workspace.people.error.cannotRemove')}
/>
<View style={styles.flex1}>
<OptionRow
onSelectRow={() => this.toggleUser(item.login)}
forceTextUnreadStyle
isDisabled={!canBeRemoved}
option={{
text: Str.removeSMSDomain(item.displayName),
alternateText: Str.removeSMSDomain(item.login),
participantsList: [item],
icons: [item.avatar],
keyForList: item.login,
}}
activeOpacity={0.7}
>
<CheckboxWithTooltip
style={[styles.peopleRowCell]}
isChecked={_.contains(this.state.selectedEmployees, item.login)}
disabled={!canBeRemoved}
onPress={() => this.toggleUser(item.login)}
toggleTooltip={this.state.showTooltipForLogin === item.login}
text={this.props.translate('workspace.people.error.cannotRemove')}
/>
</View>
{this.props.session.email === item.login && (
<View style={styles.flex1}>
<OptionRow
onSelectRow={() => this.toggleUser(item.login)}
forceTextUnreadStyle
isDisabled={!canBeRemoved}
option={{
text: Str.removeSMSDomain(item.displayName),
alternateText: Str.removeSMSDomain(item.login),
participantsList: [item],
icons: [item.avatar],
keyForList: item.login,
}}
/>
</View>
{this.props.session.email === item.login && (
<View style={styles.peopleRowCell}>
<View style={[styles.badge, styles.peopleBadge]}>
<Text style={[styles.peopleBadgeText]}>
{this.props.translate('common.admin')}
</Text>
</View>
</View>
)}
</TouchableOpacity>
</Hoverable>
)}
</TouchableOpacity>
</Hoverable>
</OfflineWithFeedback>
);
}

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,
};
39 changes: 39 additions & 0 deletions src/styles/styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -2335,6 +2335,9 @@ const styles = {
width: '100%',
flexDirection: 'row',
justifyContent: 'space-between',
},

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

offlineFeedback: {
deleted: {
textDecorationLine: 'line-through',
textDecorationStyle: 'solid',
},
pending: {
opacity: 0.5,
},
error: {
flexDirection: 'row',
justifyContent: 'center',
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 this is unnecessary: justifyContent: 'center', but also should not cause bugs

alignItems: 'center',
},
container: {
...spacing.pv2,
},
text: {
color: themeColors.textSupporting,
flex: 1,
textAlignVertical: 'center',
},
close: {
marginTop: 'auto',
marginBottom: 'auto',
aldo-expensify marked this conversation as resolved.
Show resolved Hide resolved
},
errorDot: {
borderColor: themeColors.textError,
borderRadius: 6,
borderWidth: 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

At least in web, these border properties seem to be unnecessary (except for borderRadius )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied them from somewhere else... will do some tests in native and see if they make a difference there

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing with the error dot - I think we want to wrap it in a view that has 2px padding. This way the total bound of the red dot is 16x16, which will match other small icons we use for this kind of icon + message pattern. So essentially we'd get something like this:
image

Let me know what you think about that. I know this isn't an "icon" per se, but all of our icons have a little bit of this white space around it.

Actually, that being said, what we might want to do is just supply this as an .svg and then serve it up just like the rest of the icon system? Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am unsure. I would think is better to do this than using svgs if the effect is the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

But this would then become almost like an "exception" to the icon system, ya know?

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 ok, consistency then. I am fine with making it SVG, but have no idea how to do that, any tips?

Copy link
Contributor

Choose a reason for hiding this comment

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

I got you! dot-indicator.svg.zip

So then just make sure this shows at 16x16, and uses red (danger) for the fill color.

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 thanks! So I drop this in /assets/images?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe so, you should see a bunch of other .svgs there

height: 12,
width: 12,
marginRight: 8,
backgroundColor: themeColors.textError,
},
},

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