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.

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

Jump to bottom

Standardize form error styles #12098

Merged
merged 9 commits into from
Nov 3, 2022
Merged

Standardize form error styles #12098

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
817f637
Standardize form error styles
Oct 24, 2022
ae1a24e
update FormHelpMessage props
Oct 24, 2022
3b4f59f
add comments in FormHelpMessage propTypes
Oct 24, 2022
dcd79a6
update FormHelpMessage component from feedback
Oct 27, 2022
236d9c2
add another 4px top margin between indicator message and input
Oct 27, 2022
26e5948
update comment on FormHelpMessage
Oct 28, 2022
0477a6e
move error inside wrapper on CheckboxWithLabel component
Oct 31, 2022
8700155
add RenderHTML to storybook
Nov 1, 2022
8f004ab
Merge branch 'main' of https://github.com/0xmiroslav/Expensify into i…
Nov 3, 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
6 changes: 2 additions & 4 deletions src/components/CheckboxWithLabel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import _ from 'underscore';
import styles from '../styles/styles';
import Checkbox from './Checkbox';
import Text from './Text';
import InlineErrorText from './InlineErrorText';
import FormHelpMessage from './FormHelpMessage';

const requiredPropsCheck = (props) => {
if (!props.label && !props.LabelComponent) {
Expand Down Expand Up @@ -117,9 +117,7 @@ class CheckboxWithLabel extends React.Component {
{this.LabelComponent && (<this.LabelComponent />)}
</TouchableOpacity>
</View>
<InlineErrorText styles={[styles.ml8]}>
{this.props.errorText}
</InlineErrorText>
<FormHelpMessage message={this.props.errorText} />
</>
);
}
Expand Down
66 changes: 30 additions & 36 deletions src/components/FormAlertWrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,13 @@ import {View} from 'react-native';
import PropTypes from 'prop-types';
import React from 'react';
import {withNetwork} from './OnyxProvider';
import Icon from './Icon';
import * as Expensicons from './Icon/Expensicons';
import RenderHTML from './RenderHTML';
import TextLink from './TextLink';
import Text from './Text';
import colors from '../styles/colors';
import compose from '../libs/compose';
import networkPropTypes from './networkPropTypes';
import styles from '../styles/styles';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import FormHelpMessage from './FormHelpMessage';

const propTypes = {
/** Wrapped child components */
Expand Down Expand Up @@ -52,38 +49,35 @@ const defaultProps = {
//
// This component takes other components as a child prop. It will then render any wrapped components as a function using "render props",
// and passes it a (bool) isOffline parameter. Child components can then use the isOffline variable to determine offline behavior.
const FormAlertWrapper = props => (
<View style={props.containerStyles}>
{props.isAlertVisible && (
<View style={[styles.flexRow, styles.alignItemsCenter, styles.mb3]}>
<Icon src={Expensicons.Exclamation} fill={colors.red} />
<View style={[styles.flexRow, styles.ml2, styles.flexWrap, styles.flex1]}>
{!_.isEmpty(props.message) && props.isMessageHtml && <RenderHTML html={`<muted-text>${props.message}</muted-text>`} />}

{!_.isEmpty(props.message) && !props.isMessageHtml && <Text style={styles.mutedTextLabel}>{props.message}</Text>}

{_.isEmpty(props.message) && (
<>
<Text style={styles.mutedTextLabel}>
{`${props.translate('common.please')} `}
</Text>
<TextLink
style={styles.label}
onPress={props.onFixTheErrorsLinkPressed}
>
{props.translate('common.fixTheErrors')}
</TextLink>
<Text style={styles.mutedTextLabel}>
{` ${props.translate('common.inTheFormBeforeContinuing')}.`}
</Text>
</>
)}
</View>
</View>
)}
{props.children(props.network.isOffline)}
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'm lost. Why did we need this? Can't we just subscribe to the Onyx network key instead of rendering a child?

@marcaaron that was already existing code before this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok thanks, let's see maybe it was me who added it lol

</View>
);
const FormAlertWrapper = (props) => {
let message;
if (_.isEmpty(props.message)) {
message = (
<>
{`${props.translate('common.please')} `}
<TextLink
style={styles.label}
onPress={props.onFixTheErrorsLinkPressed}
>
{props.translate('common.fixTheErrors')}
</TextLink>
{` ${props.translate('common.inTheFormBeforeContinuing')}.`}
</>
Copy link
Member

Choose a reason for hiding this comment

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

We don't use such patterns where UI blocks are passed as props.

What you can do is?

  1. this part can be passed as children like RenderHTML.
  2. Else if part is only message which you can directly pass to the message prop.
  3. See next comment.

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 don't use such patterns where UI blocks are passed as props

@parasharrajat I did this because it shouldn't be rendered inside View but Text (to fix wrap issue in #11410)
So what I suggest is that remove message prop and use only children. But instead introduce useDirectChildren prop whether to put children inside Text or not
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

useDirectChildren just feels confusing to me. It will become difficult for others to reason whether to use useDirectChildren or not in the future.

Instead, the message clearly indicates that it accepts simple text. Only a few cases will require using the children varient if the user has to put in complex rendering logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat updated from your feedback and pushed

);
} else if (!props.isMessageHtml) {
message = props.message;
}
return (
<View style={props.containerStyles}>
{props.isAlertVisible && (
<FormHelpMessage message={message} style={[styles.mb3]}>
{_.isEmpty(message) && <RenderHTML html={`<muted-text>${props.message}</muted-text>`} />}
</FormHelpMessage>
)}
{props.children(props.network.isOffline)}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm lost. Why did we need this? Can't we just subscribe to the Onyx network key instead of rendering a child?

</View>
);
};

FormAlertWrapper.propTypes = propTypes;
FormAlertWrapper.defaultProps = defaultProps;
Expand Down
50 changes: 50 additions & 0 deletions src/components/FormHelpMessage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import React from 'react';
import _ from 'underscore';
import PropTypes from 'prop-types';
import {View} from 'react-native';
import styles from '../styles/styles';
import Icon from './Icon';
import * as Expensicons from './Icon/Expensicons';
import colors from '../styles/colors';
import Text from './Text';

const propTypes = {
message: PropTypes.node,

children: PropTypes.node,

isError: PropTypes.bool,

// eslint-disable-next-line react/forbid-prop-types
style: PropTypes.arrayOf(PropTypes.object),
Copy link
Member

Choose a reason for hiding this comment

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

Missing comments. Check styleGuide.

Copy link
Member

Choose a reason for hiding this comment

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

Please use stylePropTypes via import.

};

const defaultProps = {
message: null,
children: null,
isError: true,
style: [],
};

const FormHelpMessage = (props) => {
if (_.isEmpty(props.message) && _.isEmpty(props.children)) {
return null;
}

return (
<View style={[styles.formHelpMessage, styles.mt1, styles.mb1, ...props.style]}>
{props.isError && <Icon src={Expensicons.DotIndicator} fill={colors.red} />}
{!_.isEmpty(props.message) ? (
<Text style={[props.isError ? styles.formError : styles.formHelp, styles.flex1, styles.mb0, styles.ml2]}>
{props.message}
</Text>
) : <View style={[styles.flex1, styles.ml2]}>{props.children}</View>}
Copy link
Member

Choose a reason for hiding this comment

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

Now reverse this condition. Children take priority. When children is passed override the props.message's Text block.

</View>
);
};

FormHelpMessage.propTypes = propTypes;
FormHelpMessage.defaultProps = defaultProps;
FormHelpMessage.displayName = 'FormHelpMessage';

export default FormHelpMessage;
6 changes: 2 additions & 4 deletions src/components/Picker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import PropTypes from 'prop-types';
import BasePicker from './BasePicker';
import Text from '../Text';
import styles from '../../styles/styles';
import InlineErrorText from '../InlineErrorText';
import FormHelpMessage from '../FormHelpMessage';

const propTypes = {
/** Picker label */
Expand Down Expand Up @@ -95,9 +95,7 @@ class Picker extends PureComponent {
onInputChange={this.onInputChange}
/>
</View>
<InlineErrorText styles={[styles.mh3]}>
{this.props.errorText}
</InlineErrorText>
<FormHelpMessage message={this.props.errorText} />
</>
);
}
Expand Down
6 changes: 2 additions & 4 deletions src/components/RadioButtonWithLabel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import _ from 'underscore';
import styles from '../styles/styles';
import RadioButton from './RadioButton';
import Text from './Text';
import InlineErrorText from './InlineErrorText';
import FormHelpMessage from './FormHelpMessage';

const propTypes = {
/** Whether the radioButton is checked */
Expand Down Expand Up @@ -75,9 +75,7 @@ const RadioButtonWithLabel = (props) => {
{LabelComponent && (<LabelComponent />)}
</TouchableOpacity>
</View>
<InlineErrorText>
{props.errorText}
</InlineErrorText>
<FormHelpMessage message={props.errorText} />
</>
);
};
Expand Down
6 changes: 2 additions & 4 deletions src/components/TextInput/BaseTextInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import variables from '../../styles/variables';
import Checkbox from '../Checkbox';
import getSecureEntryKeyboardType from '../../libs/getSecureEntryKeyboardType';
import CONST from '../../CONST';
import FormHelpMessage from '../FormHelpMessage';

class BaseTextInput extends Component {
constructor(props) {
Expand Down Expand Up @@ -198,7 +199,6 @@ class BaseTextInput extends Component {
const inputProps = _.omit(this.props, _.keys(baseTextInputPropTypes.propTypes));
const hasLabel = Boolean(this.props.label.length);
const inputHelpText = this.props.errorText || this.props.hint;
const formHelpStyles = this.props.errorText ? styles.formError : styles.formHelp;
const placeholder = (this.props.prefixCharacter || this.state.isFocused || !hasLabel || (hasLabel && this.props.forceActiveLabel)) ? this.props.placeholder : null;
const textInputContainerStyles = _.reduce([
styles.textInputContainer,
Expand Down Expand Up @@ -305,9 +305,7 @@ class BaseTextInput extends Component {
</TouchableWithoutFeedback>
</View>
{!_.isEmpty(inputHelpText) && (
<Text style={[formHelpStyles, styles.mt1, styles.ph3]}>
{inputHelpText}
</Text>
<FormHelpMessage isError={!_.isEmpty(this.props.errorText)} message={inputHelpText} />
)}
</View>
{/*
Expand Down
6 changes: 6 additions & 0 deletions src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,6 +869,12 @@ const styles = {
marginBottom: 8,
},

formHelpMessage: {
display: 'flex',
flexDirection: 'row',
alignItems: 'center',
},
Copy link
Member

Choose a reason for hiding this comment

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

Please use style util objects. e.g. styles.flexRow etc.


formHelp: {
color: themeColors.textSupporting,
fontSize: variables.fontSizeLabel,
Expand Down