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

statepicker refactored to compatible with form #8758

Merged
merged 11 commits into from
Apr 28, 2022
52 changes: 43 additions & 9 deletions src/components/StatePicker.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import _ from 'underscore';
import React from 'react';
import React, {forwardRef} from 'react';
import PropTypes from 'prop-types';
import {CONST} from 'expensify-common/lib/CONST';
import Picker from './Picker';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import * as FormUtils from '../libs/FormUtils';

const STATES = _.map(CONST.STATES, ({stateISO}) => ({
value: stateISO,
Expand All @@ -14,31 +15,64 @@ const propTypes = {
/** The label for the field */
label: PropTypes.string,

/** A callback method that is called when the value changes and it received the selected value as an argument */
onChange: PropTypes.func.isRequired,
/** A callback method that is called when the value changes and it receives the selected value as an argument. */
onInputChange: PropTypes.func.isRequired,

/** The value that needs to be selected */
value: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),

/** Indicates that the input is being used with the Form component */
isFormInput: PropTypes.bool,

/**
* The ID used to uniquely identify the input
*
* @param {Object} props - props passed to the input
* @returns {Object} - returns an Error object if isFormInput is supplied but inputID is falsey or not a string
*/
inputID: props => FormUtils.validateInputIDProps(props),

/** Saves a draft of the input value when used in a form */
shouldSaveDraft: PropTypes.bool,

/** Callback that is called when the text input is blurred */
onBlur: PropTypes.func,

/** Error text to display */
errorText: PropTypes.string,

/** The default value of the state picker */
defaultValue: PropTypes.string,

...withLocalizePropTypes,
};

const defaultProps = {
label: '',
value: '',
value: undefined,
defaultValue: undefined,
errorText: '',
shouldSaveDraft: false,
isFormInput: false,
inputID: undefined,
onBlur: () => {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I see this console warning:
warn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's solved now, now tested the app in the web and no error is there and picker is working

Screenshot 2022-04-28 at 11 14 19 AM

tested in storybook , its working fine and no console error

storybook-test.mov

};

const StatePicker = props => (
const StatePicker = forwardRef((props, ref) => (
<Picker
ref={ref}
inputID={props.inputID}
placeholder={{value: '', label: '-'}}
items={STATES}
onInputChange={props.onChange}
value={props.value}
onInputChange={props.onInputChange}
value={props.value ? props.value : props.defaultValue}
label={props.label || props.translate('common.state')}
hasError={props.hasError}
errorText={props.errorText}
onBlur={props.onBlur}
isFormInput={props.isFormInput}
shouldSaveDraft={props.shouldSaveDraft}
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
/>
);
));

StatePicker.propTypes = propTypes;
StatePicker.defaultProps = defaultProps;
Expand Down
3 changes: 1 addition & 2 deletions src/pages/ReimbursementAccount/AddressForm.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,8 @@ const AddressForm = props => (
<View style={[styles.flex1]}>
<StatePicker
value={props.values.state}
onChange={value => props.onFieldChange({state: value})}
onInputChange={value => props.onFieldChange({state: value})}
errorText={props.errors.state ? props.translate('bankAccount.error.addressState') : ''}
hasError={Boolean(props.errors.state)}
/>
</View>
</View>
Expand Down
5 changes: 3 additions & 2 deletions src/pages/ReimbursementAccount/CompanyStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ class CompanyStep extends React.Component {
incorporationDateFuture: 'bankAccount.error.incorporationDateFuture',
incorporationType: 'bankAccount.error.companyType',
hasNoConnectionToCannabis: 'bankAccount.error.restrictedBusiness',
incorporationState: 'bankAccount.error.incorporationState',
};

this.getErrorText = inputKey => ReimbursementAccountUtils.getErrorText(this.props, this.errorTranslationKeys, inputKey);
Expand Down Expand Up @@ -286,9 +287,9 @@ class CompanyStep extends React.Component {
<View style={styles.mt4}>
<StatePicker
label={this.props.translate('companyStep.incorporationState')}
onChange={value => this.clearErrorAndSetValue('incorporationState', value)}
onInputChange={value => this.clearErrorAndSetValue('incorporationState', value)}
value={this.state.incorporationState}
hasError={this.getErrors().incorporationState}
errorText={this.getErrorText('incorporationState')}
Copy link
Contributor

Choose a reason for hiding this comment

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

The incorporationState key is not defined here.

/>
</View>
<CheckboxWithLabel
Expand Down
4 changes: 2 additions & 2 deletions src/pages/settings/Payments/AddDebitCardPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -299,9 +299,9 @@ class DebitCardPage extends Component {
</View>
<View style={[styles.flex1]}>
<StatePicker
onChange={value => this.clearErrorAndSetValue('addressState', value)}
onInputChange={value => this.clearErrorAndSetValue('addressState', value)}
value={this.state.addressState}
hasError={lodashGet(this.state.errors, 'addressState', false)}
errorText={this.getErrorText('addressState')}
/>
</View>
</View>
Expand Down
14 changes: 14 additions & 0 deletions src/stories/Form.stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, {useState} from 'react';
import {View} from 'react-native';
import TextInput from '../components/TextInput';
import Picker from '../components/Picker';
import StatePicker from '../components/StatePicker';
import AddressSearch from '../components/AddressSearch';
import DatePicker from '../components/DatePicker';
import Form from '../components/Form';
Expand All @@ -23,6 +24,7 @@ const story = {
AddressSearch,
CheckboxWithLabel,
Picker,
StatePicker,
DatePicker,
},
};
Expand Down Expand Up @@ -105,6 +107,13 @@ const Template = (args) => {
]}
isFormInput
/>
<View style={styles.mt4}>
<StatePicker
inputID="pickState"
shouldSaveDraft
isFormInput
/>
</View>
<CheckboxWithLabel
inputID="checkbox"
style={[styles.mb4, styles.mt5]}
Expand Down Expand Up @@ -175,6 +184,9 @@ const defaultArgs = {
if (!values.pickAnotherFruit) {
errors.pickAnotherFruit = 'Please select a fruit';
}
if (!values.pickState) {
errors.pickState = 'Please select a state';
}
if (!values.checkbox) {
errors.checkbox = 'You must accept the Terms of Service to continue';
}
Expand All @@ -196,6 +208,7 @@ const defaultArgs = {
dob: '1990-01-01',
pickFruit: 'orange',
pickAnotherFruit: 'apple',
pickState: 'AL',
checkbox: false,
},
};
Expand All @@ -211,6 +224,7 @@ InputError.args = {
pickFruit: '',
dob: '',
pickAnotherFruit: '',
pickState: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add a non-empty default value for pickState here?

During my testing, the default value didn't seem to work properly on page load.

storybook.mov

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 think we have to add 1 more prop in StatePicker for this change, we need to pass defaultValue to make it work

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 have added a default value for pickState, but we also have to pass defaultValue in StatePicker component, please let me know your thought on this

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds similar to what I'm doing in the Datepicker refactor. If that's the case I think it's fine :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, then its fine

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 default value still doesn't load for me. Can you please check?

default.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checkbox: false,
},
};
Expand Down