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
42 changes: 34 additions & 8 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,56 @@ 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,

...withLocalizePropTypes,
};

const defaultProps = {
label: '',
value: '',
value: undefined,
errorText: '',
shouldSaveDraft: false,
};

const StatePicker = props => (
const StatePicker = forwardRef((props, ref) => (
<Picker
ref={ref}
placeholder={{value: '', label: '-'}}
items={STATES}
onInputChange={props.onChange}
onInputChange={props.onInputChange}
value={props.value}
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: 2 additions & 1 deletion src/pages/ReimbursementAccount/CompanyStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +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}
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.

hasError={this.getErrors().incorporationState}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we remove these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out these changes, Yes I need to change all these places where we are using StatePicker,

hasError={Boolean(props.errors.state)}

hasError={this.getErrors().incorporationState}

/>
</View>
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
10 changes: 10 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 Form from '../components/Form';
import * as FormActions from '../libs/actions/FormActions';
Expand Down Expand Up @@ -97,6 +98,11 @@ const Template = (args) => {
]}
isFormInput
/>
<StatePicker
inputID="pickState"
shouldSaveDraft
isFormInput
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a top margin like the other inputs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have not defined a style prop for StatePicker, we need to create containerStyles prop here

Copy link
Contributor

@luacmartins luacmartins Apr 25, 2022

Choose a reason for hiding this comment

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

or you can just wrap it in a View like we do in all calls of StatePicker, e.g.

<View style={styles.mt4}>
<StatePicker
label={this.props.translate('companyStep.incorporationState')}
onChange={value => this.clearErrorAndSetValue('incorporationState', value)}
value={this.state.incorporationState}
hasError={this.getErrors().incorporationState}
/>
</View>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohkay

<CheckboxWithLabel
inputID="checkbox"
style={[styles.mb4, styles.mt5]}
Expand Down Expand Up @@ -164,6 +170,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 Down Expand Up @@ -198,6 +207,7 @@ InputError.args = {
accountNumber: '',
pickFruit: '',
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