Skip to content

Clean up CheckboxWithLabel refactor #8682

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.

Sign up for GitHub

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

Merged
Luke9389 merged 5 commits into from
Apr 20, 2022
Merged

Clean up CheckboxWithLabel refactor #8682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f837637
rm default value and event handler from form stories
luacmartins Apr 18, 2022
ce5d2e1
rename onChange in usages of CheckboxWithLabel
luacmartins Apr 18, 2022
f0f0970
use local isChecked
luacmartins Apr 18, 2022
e3537b0
declare and spread props
luacmartins Apr 18, 2022
187aa68
add comment
luacmartins Apr 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 24 additions & 13 deletions src/components/CheckboxWithLabel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,10 @@ import * as FormUtils from '../libs/FormUtils';

const propTypes = {
/** Whether the checkbox is checked */
isChecked: PropTypes.bool.isRequired,
isChecked: PropTypes.bool,

/** Called when the checkbox or label is pressed */
onPress: PropTypes.func.isRequired,

/** Called when the checkbox or label is pressed */
onChange: PropTypes.func,
onInputChange: PropTypes.func.isRequired,

/** Container styles */
style: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]),
Expand All @@ -33,6 +30,12 @@ const propTypes = {
/** Indicates that the input is being used with the Form component */
isFormInput: PropTypes.bool,

/** The default value for the checkbox */
defaultValue: PropTypes.bool,

/** React ref being forwarded to the Checkbox input */
forwardedRef: PropTypes.func,

/**
* The ID used to uniquely identify the input
*
Expand All @@ -46,24 +49,29 @@ const propTypes = {
};

const defaultProps = {
onChange: () => {},
isFormInput: false,
inputID: undefined,
style: [],
label: undefined,
LabelComponent: undefined,
errorText: '',
shouldSaveDraft: false,
isChecked: false,
defaultValue: false,
forwardedRef: () => {},
};

const CheckboxWithLabel = React.forwardRef((props, ref) => {
const CheckboxWithLabel = (props) => {
const LabelComponent = props.LabelComponent;
const defaultStyles = [styles.flexRow, styles.alignItemsCenter];
const wrapperStyles = _.isArray(props.style) ? [...defaultStyles, ...props.style] : [...defaultStyles, props.style];

// We keep track of the checkbox "state" in a local variable so that this component has an uncontrolled input interface
let isChecked = props.defaultValue ? props.defaultValue : props.isChecked;
Copy link
Contributor

@aimane-chnaif aimane-chnaif May 22, 2023

Choose a reason for hiding this comment

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

This condition caused issue - #16885.
As checkbox value is boolean, we cannot use ? if defaultValue is false.
So we fixed this logic in #18479 by strictly checking boolean type.


function toggleCheckbox() {
props.onPress(!props.isChecked);
props.onChange(!props.isChecked);
props.onInputChange(!isChecked);
isChecked = !isChecked;
}

if (!props.label && !LabelComponent) {
Expand All @@ -73,11 +81,11 @@ const CheckboxWithLabel = React.forwardRef((props, ref) => {
<>
<View style={wrapperStyles}>
<Checkbox
isChecked={props.isChecked}
isChecked={isChecked}
onPress={toggleCheckbox}
label={props.label}
hasError={Boolean(props.errorText)}
forwardedRef={ref}
forwardedRef={props.forwardedRef}
isFormInput={props.isFormInput}
inputID={props.inputID}
shouldSaveDraft={props.shouldSaveDraft}
Expand Down Expand Up @@ -107,10 +115,13 @@ const CheckboxWithLabel = React.forwardRef((props, ref) => {
</InlineErrorText>
</>
);
});
};

CheckboxWithLabel.propTypes = propTypes;
CheckboxWithLabel.defaultProps = defaultProps;
CheckboxWithLabel.displayName = 'CheckboxWithLabel';

export default CheckboxWithLabel;
export default React.forwardRef((props, ref) => (
// eslint-disable-next-line react/jsx-props-no-spreading
<CheckboxWithLabel {...props} forwardedRef={ref} />
));
4 changes: 2 additions & 2 deletions src/pages/EnablePayments/TermsStep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class TermsStep extends React.Component {
<CheckboxWithLabel
style={[styles.mb4, styles.mt4]}
isChecked={this.state.hasAcceptedDisclosure}
onPress={this.toggleDisclosure}
onInputChange={this.toggleDisclosure}
LabelComponent={() => (
<Text>
{`${this.props.translate('termsStep.haveReadAndAgree')}`}
Expand All @@ -88,7 +88,7 @@ class TermsStep extends React.Component {
/>
<CheckboxWithLabel
isChecked={this.state.hasAcceptedPrivacyPolicyAndWalletAgreement}
onPress={this.togglePrivacyPolicy}
onInputChange={this.togglePrivacyPolicy}
LabelComponent={() => (
<>
<Text>
Expand Down
8 changes: 4 additions & 4 deletions src/pages/ReimbursementAccount/ACHContractStep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ class ACHContractStep extends React.Component {
<CheckboxWithLabel
style={[styles.mb2]}
isChecked={this.state.ownsMoreThan25Percent}
onPress={() => this.toggleCheckbox('ownsMoreThan25Percent')}
onInputChange={() => this.toggleCheckbox('ownsMoreThan25Percent')}
LabelComponent={() => (
<Text>
{this.props.translate('beneficialOwnersStep.iOwnMoreThan25Percent')}
Expand All @@ -196,7 +196,7 @@ class ACHContractStep extends React.Component {
<CheckboxWithLabel
style={[styles.mb2]}
isChecked={this.state.hasOtherBeneficialOwners}
onPress={() => {
onInputChange={() => {
this.setState((prevState) => {
const hasOtherBeneficialOwners = !prevState.hasOtherBeneficialOwners;
const newState = {
Expand Down Expand Up @@ -259,7 +259,7 @@ class ACHContractStep extends React.Component {
<CheckboxWithLabel
style={[styles.mt4]}
isChecked={this.state.acceptTermsAndConditions}
onPress={() => this.toggleCheckbox('acceptTermsAndConditions')}
onInputChange={() => this.toggleCheckbox('acceptTermsAndConditions')}
LabelComponent={() => (
<View style={[styles.flexRow]}>
<Text>{this.props.translate('common.iAcceptThe')}</Text>
Expand All @@ -274,7 +274,7 @@ class ACHContractStep extends React.Component {
<CheckboxWithLabel
style={[styles.mt4]}
isChecked={this.state.certifyTrueInformation}
onPress={() => this.toggleCheckbox('certifyTrueInformation')}
onInputChange={() => this.toggleCheckbox('certifyTrueInformation')}
LabelComponent={() => (
<Text>{this.props.translate('beneficialOwnersStep.certifyTrueAndAccurate')}</Text>
)}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/ReimbursementAccount/BankAccountStep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ class BankAccountStep extends React.Component {
<CheckboxWithLabel
style={styles.mt4}
isChecked={this.state.hasAcceptedTerms}
onPress={this.toggleTerms}
onInputChange={this.toggleTerms}
LabelComponent={() => (
<View style={[styles.flexRow, styles.alignItemsCenter]}>
<Text>
Expand Down
2 changes: 1 addition & 1 deletion src/pages/ReimbursementAccount/CompanyStep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ class CompanyStep extends React.Component {
</View>
<CheckboxWithLabel
isChecked={this.state.hasNoConnectionToCannabis}
onPress={() => {
onInputChange={() => {
this.setState((prevState) => {
const newState = {hasNoConnectionToCannabis: !prevState.hasNoConnectionToCannabis};
BankAccounts.updateReimbursementAccountDraft(newState);
Expand Down
2 changes: 1 addition & 1 deletion src/pages/ReimbursementAccount/RequestorStep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ class RequestorStep extends React.Component {
/>
<CheckboxWithLabel
isChecked={this.state.isControllingOfficer}
onPress={() => {
onInputChange={() => {
this.setState((prevState) => {
const newState = {isControllingOfficer: !prevState.isControllingOfficer};
BankAccounts.updateReimbursementAccountDraft(newState);
Expand Down
2 changes: 1 addition & 1 deletion src/pages/settings/Payments/AddDebitCardPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ class DebitCardPage extends Component {
</View>
<CheckboxWithLabel
isChecked={this.state.acceptedTerms}
onPress={() => {
onInputChange={() => {
this.setState(prevState => ({
acceptedTerms: !prevState.acceptedTerms,
errors: {
Expand Down
2 changes: 1 addition & 1 deletion src/pages/settings/Profile/ProfilePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ class ProfilePage extends Component {
<CheckboxWithLabel
label={this.props.translate('profilePage.setMyTimezoneAutomatically')}
isChecked={this.state.isAutomaticTimezone}
onPress={this.setAutomaticTimezone}
onInputChange={this.setAutomaticTimezone}
/>
</ScrollView>
<FixedFooter>
Expand Down
5 changes: 0 additions & 5 deletions src/stories/Form.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ const story = {
};

const Template = (args) => {
const [isChecked, setIsChecked] = useState(args.draftValues.checkbox);

// Form consumes data from Onyx, so we initialize Onyx with the necessary data here
FormActions.setIsSubmitting(args.formID, args.formState.isSubmitting);
FormActions.setServerErrorMessage(args.formID, args.formState.serverErrorMessage);
Expand Down Expand Up @@ -101,10 +99,7 @@ const Template = (args) => {
/>
<CheckboxWithLabel
inputID="checkbox"
isChecked={isChecked}
defaultValue={isChecked}
style={[styles.mb4, styles.mt5]}
onPress={() => { setIsChecked(prev => !prev); }}
isFormInput
shouldSaveDraft
LabelComponent={() => (
Expand Down