Skip to content

Commit

Permalink
Merge pull request #6234 from Expensify/Rory-NoPropsDestructuring
Browse files Browse the repository at this point in the history
[No QA] No Props/State Destructuring
  • Loading branch information
marcaaron authored Nov 8, 2021
2 parents 180c7e7 + be757dc commit b88455e
Show file tree
Hide file tree
Showing 90 changed files with 770 additions and 1,000 deletions.
22 changes: 14 additions & 8 deletions STYLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,7 @@ const {name, accountID, email} = data;

**React Components**

- Avoid destructuring props and state at the *same time*. It makes the source of a given variable unclear.
- Avoid destructuring either props or state when there are other variables declared in a render block. This helps us quickly know which variables are from props, state, or declared inside the render.
- Use parameter destructuring for stateless function components when there are no additional variable declarations in the render.
Don't destructure props or state. It makes the source of a given variable unclear. This guideline helps us quickly know which variables are from props, state, or from some other scope.
```javascript
// Bad
Expand All @@ -276,12 +274,20 @@ render() {
...
}
// Good
// Bad
const UserInfo = ({name, email}) => (
<div>
<p>Name: {name}</p>
<p>Email: {email}</p>
</div>
<View>
<Text>Name: {name}</Text>
<Text>Email: {email}</Text>
</View>
);
// Good
const UserInfo = props => (
<View>
<Text>Name: {props.name}</Text>
<Text>Email: {props.email}</Text>
</View>
);
```
Expand Down
6 changes: 3 additions & 3 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@
"electron-notarize": "^1.0.0",
"electron-reloader": "^1.2.0",
"eslint": "^7.6.0",
"eslint-config-expensify": "2.0.17",
"eslint-config-expensify": "^2.0.18",
"eslint-loader": "^4.0.2",
"eslint-plugin-detox": "^1.0.0",
"eslint-plugin-jest": "^24.1.0",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import _ from 'underscore';
import React from 'react';
import {Pressable, StyleSheet} from 'react-native';
import lodashGet from 'lodash/get';
Expand All @@ -9,31 +10,22 @@ import {CONTEXT_MENU_TYPES} from '../../../pages/home/report/ContextMenu/Context
import AttachmentView from '../../AttachmentView';
import fileDownload from '../../../libs/fileDownload';


/*
* This is a default anchor component for regular links.
*/
const BaseAnchorForCommentsOnly = ({
href,
rel,
target,
children,
style,
fileName,
...props
}) => {
const BaseAnchorForCommentsOnly = (props) => {
let linkRef;
const rest = _.omit(props, _.keys(propTypes));
return (

props.isAttachment
? (
<Pressable onPress={() => {
fileDownload(href, fileName);
fileDownload(props.href, props.fileName);
}}
>
<AttachmentView
sourceURL={href}
file={{name: fileName}}
sourceURL={props.href}
file={{name: props.fileName}}
shouldShowDownloadIcon
/>
</Pressable>
Expand All @@ -45,22 +37,25 @@ const BaseAnchorForCommentsOnly = ({
showContextMenu(
CONTEXT_MENU_TYPES.LINK,
event,
href,
props.href,
lodashGet(linkRef, 'current'),
);
}
}
>
<Text
ref={el => linkRef = el}
style={StyleSheet.flatten(style)}
style={StyleSheet.flatten(props.style)}
accessibilityRole="link"
href={href}
hrefAttrs={{rel, target}}
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
href={props.href}
hrefAttrs={{
rel: props.rel,
target: props.target,
}}
// eslint-disable-next-line react/jsx-props-no-spreading
{...rest}
>
{children}
{props.children}
</Text>
</PressableWithSecondaryInteraction>
)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import _ from 'underscore';
import React from 'react';
import lodashGet from 'lodash/get';
import {Linking, StyleSheet, Pressable} from 'react-native';
Expand All @@ -13,27 +14,21 @@ import styles from '../../../styles/styles';
/*
* This is a default anchor component for regular links.
*/
const BaseAnchorForCommentsOnly = ({
href,
children,
style,
isAttachment,
fileName,
...props
}) => {
const BaseAnchorForCommentsOnly = (props) => {
let linkRef;
const rest = _.omit(props, _.keys(propTypes));
return (
isAttachment
props.isAttachment
? (
<Pressable
style={styles.mw100}
onPress={() => {
fileDownload(href, fileName);
fileDownload(props.href, props.fileName);
}}
>
<AttachmentView
sourceURL={href}
file={{name: fileName}}
sourceURL={props.href}
file={{name: props.fileName}}
shouldShowDownloadIcon
/>
</Pressable>
Expand All @@ -45,20 +40,20 @@ const BaseAnchorForCommentsOnly = ({
showContextMenu(
CONTEXT_MENU_TYPES.LINK,
event,
href,
props.href,
lodashGet(linkRef, 'current'),
);
}
}
onPress={() => Linking.openURL(href)}
onPress={() => Linking.openURL(props.href)}
>
<Text
ref={el => linkRef = el}
style={StyleSheet.flatten(style)}
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
style={StyleSheet.flatten(props.style)}
// eslint-disable-next-line react/jsx-props-no-spreading
{...rest}
>
{children}
{props.children}
</Text>
</PressableWithSecondaryInteraction>
)
Expand Down
2 changes: 1 addition & 1 deletion src/components/AvatarWithImagePicker.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ class AvatarWithImagePicker extends React.Component {
}

render() {
const {DefaultAvatar} = this.props;
const DefaultAvatar = this.props.DefaultAvatar;
const additionalStyles = _.isArray(this.props.style) ? this.props.style : [this.props.style];

const indicatorStyles = [
Expand Down
4 changes: 2 additions & 2 deletions src/components/BigNumberPad.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const padNumbers = [
['.', '0', '<'],
];

const BigNumberPad = ({numberPressed}) => (
const BigNumberPad = props => (
<View style={[styles.flexColumn, styles.w100]}>
{_.map(padNumbers, (row, rowIndex) => (
<View key={`NumberPadRow-${rowIndex}`} style={[styles.flexRow, styles.mt3]}>
Expand All @@ -30,7 +30,7 @@ const BigNumberPad = ({numberPressed}) => (
key={column}
style={[styles.flex1, marginLeft]}
text={column}
onPress={() => numberPressed(column)}
onPress={() => props.numberPressed(column)}
/>
);
})}
Expand Down
2 changes: 1 addition & 1 deletion src/components/Button.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class Button extends Component {
}

renderContent() {
const {ContentComponent} = this.props;
const ContentComponent = this.props.ContentComponent;
if (ContentComponent) {
return <ContentComponent />;
}
Expand Down
17 changes: 6 additions & 11 deletions src/components/Checkbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,22 +24,17 @@ const defaultProps = {
disabled: false,
};

const Checkbox = ({
isChecked,
onPress,
hasError,
disabled,
}) => (
const Checkbox = props => (
<Pressable
disabled={disabled}
onPress={() => onPress(!isChecked)}
disabled={props.disabled}
onPress={() => props.onPress(!props.isChecked)}
>
<View
style={[
styles.checkboxContainer,
isChecked && styles.checkedContainer,
hasError && styles.borderColorDanger,
disabled && styles.cursorDisabled,
props.isChecked && styles.checkedContainer,
props.hasError && styles.borderColorDanger,
props.disabled && styles.cursorDisabled,
]}
>
<Icon src={Checkmark} fill="white" height={14} width={14} />
Expand Down
27 changes: 13 additions & 14 deletions src/components/CheckboxWithLabel.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,26 +38,25 @@ const defaultProps = {
errorText: '',
};

const CheckboxWithLabel = ({
LabelComponent, isChecked, onPress, style, label, hasError, errorText,
}) => {
const CheckboxWithLabel = (props) => {
const LabelComponent = props.LabelComponent;
const defaultStyles = [styles.flexRow, styles.alignItemsCenter];
const wrapperStyles = _.isArray(style) ? [...defaultStyles, ...style] : [...defaultStyles, style];
const wrapperStyles = _.isArray(props.style) ? [...defaultStyles, ...props.style] : [...defaultStyles, props.style];

if (!label && !LabelComponent) {
if (!props.label && !LabelComponent) {
throw new Error('Must provide at least label or LabelComponent prop');
}
return (
<>
<View style={wrapperStyles}>
<Checkbox
isChecked={isChecked}
onPress={() => onPress(!isChecked)}
label={label}
hasError={hasError}
isChecked={props.isChecked}
onPress={() => props.onPress(!props.isChecked)}
label={props.label}
hasError={props.hasError}
/>
<TouchableOpacity
onPress={() => onPress(!isChecked)}
onPress={() => props.onPress(!props.isChecked)}
style={[
styles.ml3,
styles.pr2,
Expand All @@ -68,17 +67,17 @@ const CheckboxWithLabel = ({
styles.alignItemsCenter,
]}
>
{label && (
{props.label && (
<Text style={[styles.ml2]}>
{label}
{props.label}
</Text>
)}
{LabelComponent && (<LabelComponent />)}
</TouchableOpacity>
</View>
{!_.isEmpty(errorText) && (
{!_.isEmpty(props.errorText) && (
<InlineErrorText>
{errorText}
{props.errorText}
</InlineErrorText>
)}
</>
Expand Down
29 changes: 9 additions & 20 deletions src/components/DatePicker/index.android.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,36 +38,25 @@ class DatePicker extends React.Component {
}

render() {
const {
value,
label,
placeholder,
hasError,
errorText,
translateX,
containerStyles,
disabled,
} = this.props;

const dateAsText = value ? moment(value).format(CONST.DATE.MOMENT_FORMAT_STRING) : '';
const dateAsText = this.props.value ? moment(this.props.value).format(CONST.DATE.MOMENT_FORMAT_STRING) : '';

return (
<>
<ExpensiTextInput
label={label}
label={this.props.label}
value={dateAsText}
placeholder={placeholder}
hasError={hasError}
errorText={errorText}
containerStyles={containerStyles}
translateX={translateX}
placeholder={this.props.placeholder}
hasError={this.props.hasError}
errorText={this.props.errorText}
containerStyles={this.props.containerStyles}
translateX={this.props.translateX}
onPress={this.showPicker}
editable={false}
disabled={disabled}
disabled={this.props.disabled}
/>
{this.state.isPickerVisible && (
<RNDatePicker
value={value ? moment(value).toDate() : new Date()}
value={this.props.value ? moment(this.props.value).toDate() : new Date()}
mode="date"
onChange={this.raiseDateChange}
/>
Expand Down
Loading

0 comments on commit b88455e

Please sign in to comment.