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

[Form Provider Refactor] RoomNameInput fixes #32432

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
38 changes: 28 additions & 10 deletions src/components/Form/FormProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ function FormProvider({validate, formID, shouldValidateOnBlur, shouldValidateOnC
}
FormActions.setErrorFields(formID, null);

const validateErrors = validate(values) || {};
const validateErrors = validate(trimmedStringValues) || {};
kowczarz marked this conversation as resolved.
Show resolved Hide resolved

// Validate the input for html tags. It should supercede any other error
_.each(trimmedStringValues, (inputValue, inputID) => {
Expand Down Expand Up @@ -154,6 +154,11 @@ function FormProvider({validate, formID, shouldValidateOnBlur, shouldValidateOnC
}
}
}

if (isMatch && leadingSpaceIndex === -1) {
return;
}

// Add a validation error here because it is a string value that contains HTML characters
validateErrors[inputID] = 'common.error.invalidCharacter';
});
Expand Down Expand Up @@ -224,7 +229,12 @@ function FormProvider({validate, formID, shouldValidateOnBlur, shouldValidateOnC
inputValues[inputID] = propsToParse.defaultValue;
} else if (_.isUndefined(inputValues[inputID])) {
// We want to initialize the input value if it's undefined
inputValues[inputID] = _.isUndefined(propsToParse.defaultValue) ? getInitialValueByType(propsToParse.valueType) : propsToParse.defaultValue;
const initialValue = _.isUndefined(propsToParse.defaultValue) ? getInitialValueByType(propsToParse.valueType) : propsToParse.defaultValue;

inputValues[inputID] = _.isFunction(propsToParse.valueParser) ? propsToParse.valueParser(initialValue) : initialValue;
if (_.isFunction(propsToParse.displayParser)) {
inputValues[`${inputID}ToDisplay`] = propsToParse.displayParser(initialValue);
}
}

const errorFields = lodashGet(formState, 'errorFields', {});
Expand All @@ -237,6 +247,8 @@ function FormProvider({validate, formID, shouldValidateOnBlur, shouldValidateOnC
.first()
.value() || '';

const value = !_.isUndefined(inputValues[`${inputID}ToDisplay`]) ? inputValues[`${inputID}ToDisplay`] : inputValues[inputID];

return {
...propsToParse,
ref:
Expand All @@ -249,7 +261,7 @@ function FormProvider({validate, formID, shouldValidateOnBlur, shouldValidateOnC
inputID,
key: propsToParse.key || inputID,
errorText: errors[inputID] || fieldErrorMessage,
value: inputValues[inputID],
value,
// As the text input is controlled, we never set the defaultValue prop
// as this is already happening by the value prop.
defaultValue: undefined,
Expand Down Expand Up @@ -309,13 +321,19 @@ function FormProvider({validate, formID, shouldValidateOnBlur, shouldValidateOnC
propsToParse.onBlur(event);
}
},
onInputChange: (value, key) => {
onInputChange: (inputValue, key) => {
const inputKey = key || inputID;
setInputValues((prevState) => {
const newState = {
...prevState,
[inputKey]: value,
};
const newState = _.isFunction(propsToParse.valueParser)
? {
...prevState,
[inputKey]: propsToParse.valueParser(inputValue),
[`${inputKey}ToDisplay`]: _.isFunction(propsToParse.displayParser) ? propsToParse.displayParser(inputValue) : inputValue,
}
: {
...prevState,
[inputKey]: inputValue,
};

if (shouldValidateOnChange) {
onValidate(newState);
Expand All @@ -324,11 +342,11 @@ function FormProvider({validate, formID, shouldValidateOnBlur, shouldValidateOnC
});

if (propsToParse.shouldSaveDraft) {
FormActions.setDraftValues(formID, {[inputKey]: value});
FormActions.setDraftValues(formID, {[inputKey]: _.isFunction(propsToParse.valueParser) ? propsToParse.valueParser(inputValue) : inputValue});
}

if (_.isFunction(propsToParse.onValueChange)) {
propsToParse.onValueChange(value, inputKey);
propsToParse.onValueChange(inputValue, inputKey);
}
},
};
Expand Down
4 changes: 4 additions & 0 deletions src/components/Form/InputWrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,15 @@ const propTypes = {
inputID: PropTypes.string.isRequired,
valueType: PropTypes.string,
forwardedRef: refPropTypes,
valueParser: PropTypes.func,
displayParser: PropTypes.func,
};

const defaultProps = {
forwardedRef: undefined,
valueType: 'string',
valueParser: undefined,
displayParser: undefined,
};

function InputWrapper(props) {
Expand Down
53 changes: 14 additions & 39 deletions src/components/RoomNameInput/index.js
kowczarz marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,68 +1,43 @@
import React, {useState} from 'react';
import _ from 'underscore';
import React from 'react';
import InputWrapper from '@components/Form/InputWrapper';
import TextInput from '@components/TextInput';
import useLocalize from '@hooks/useLocalize';
import getOperatingSystem from '@libs/getOperatingSystem';
import * as RoomNameInputUtils from '@libs/RoomNameInputUtils';
import CONST from '@src/CONST';
import * as roomNameInputPropTypes from './roomNameInputPropTypes';

function RoomNameInput({isFocused, autoFocus, disabled, errorText, forwardedRef, value, onBlur, onChangeText, onInputChange, shouldDelayFocus}) {
function RoomNameInput({isFocused, autoFocus, disabled, errorText, forwardedRef, onBlur, shouldDelayFocus, inputID, roomName}) {
const {translate} = useLocalize();

const [selection, setSelection] = useState();
const keyboardType = getOperatingSystem() === CONST.OS.IOS ? CONST.KEYBOARD_TYPE.ASCII_CAPABLE : CONST.KEYBOARD_TYPE.VISIBLE_PASSWORD;

/**
* Calls the onChangeText callback with a modified room name
* @param {Event} event
*/
const setModifiedRoomName = (event) => {
const roomName = event.nativeEvent.text;
const modifiedRoomName = RoomNameInputUtils.modifyRoomName(roomName);
onChangeText(modifiedRoomName);

// if custom component has onInputChange, use it to trigger changes (Form input)
if (_.isFunction(onInputChange)) {
onInputChange(modifiedRoomName);
}

// Prevent cursor jump behaviour:
// Check if newRoomNameWithHash is the same as modifiedRoomName
// If it is then the room name is valid (does not contain unallowed characters); no action required
// If not then the room name contains unvalid characters and we must adjust the cursor position manually
// Read more: https://github.com/Expensify/App/issues/12741
const oldRoomNameWithHash = value || '';
const newRoomNameWithHash = `${CONST.POLICY.ROOM_PREFIX}${roomName}`;
if (modifiedRoomName !== newRoomNameWithHash) {
const offset = modifiedRoomName.length - oldRoomNameWithHash.length;
const newSelection = {
start: selection.start + offset,
end: selection.end + offset,
};
setSelection(newSelection);
}
};
const valueParser = (innerRoomName) => RoomNameInputUtils.modifyRoomName(innerRoomName);
const displayParser = (innerRoomName) => RoomNameInputUtils.modifyRoomName(innerRoomName, true);

return (
<TextInput
<InputWrapper
InputComponent={TextInput}
inputID={inputID}
ref={forwardedRef}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the best to do here. Using InputWrapper inside RoomNameInput to wrap TextInput feels a littler over-engineered. I would rather prefer InputWrapper to wrap RoomNameInput. InputWrapper should wrap the highest-level input.

RoomNameInput > InputWrapper > TextInput
InputWrapper > RoomNameInput > TextInput

PS: We also have a problem with that structure on NewDatePicker

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 would look better if we have moved the InputWrapper outside the component, but that would require dividing the logic of RoomNameInput between the component and screens where it's used. Since the logic of room input is quite complex and the current refactor has already caused some regressions in this PR I would only migrate the input to FormProvider. As soon as we will be sure the current changes won't introduce any regressions I would refactor the RoomNameInput according your suggestions in a separate PR.

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 trying to get rid of the valueParser and displayParser that we are passing to the Form. The FromProvider should not handle those. We should instead call the onInputChange in the RoomNameInput and supply the modified value.

This approach should result in the least code changes.

but that would require dividing the logic of RoomNameInput between the component and screens

Can you please elaborate on this one?

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 trying to get rid of the valueParser and displayParser that we are passing to the Form. The FromProvider should not handle those. We should instead call the onInputChange in the RoomNameInput and supply the modified value.

That would be great, but since we need to display different value than the one that is sent to Onyx, I couldn't find any better solution.

The second part I managed to move, I didn't spot that I forgot to move some props.

Copy link
Contributor

Choose a reason for hiding this comment

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

we need to display different value than the one that is sent to Onyx

This is already handled within <RoomNameInput />: value={value.substring(1)}.

Let's consider this scenario:

  1. Revert all the changes in this PR
  2. In places that we use RoomNameInput (i.e. RoomNamePage and WorkspaceNewRoomPage), replace <RoomNameInput ... /> with <InputWrapper InputComponent={RoomNameInput} ... />
  3. What could go wrong in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s77rt Ok, I reverted some changes, I think now it should look fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. This is looking much better. I still see some changes to src/components/RoomNameInput/ that does not look needed. And in src/libs/RoomNameInputUtils.ts too Can you please check again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do it tomorrow

disabled={disabled}
label={translate('newRoomPage.roomName')}
accessibilityLabel={translate('newRoomPage.roomName')}
role={CONST.ACCESSIBILITY_ROLE.TEXT}
prefixCharacter={CONST.POLICY.ROOM_PREFIX}
placeholder={translate('newRoomPage.social')}
onChange={setModifiedRoomName}
value={value.substring(1)} // Since the room name always starts with a prefix, we omit the first character to avoid displaying it twice.
selection={selection}
onSelectionChange={(event) => setSelection(event.nativeEvent.selection)}
errorText={errorText}
valueParser={valueParser}
displayParser={displayParser}
autoCapitalize="none"
onBlur={(event) => isFocused && onBlur(event)}
shouldDelayFocus={shouldDelayFocus}
autoFocus={isFocused && autoFocus}
maxLength={CONST.REPORT.MAX_ROOM_NAME_LENGTH}
defaultValue={roomName}
spellCheck={false}
shouldInterceptSwipe
keyboardType={keyboardType} // this is a bit hacky solution to a RN issue https://github.com/facebook/react-native/issues/27449
/>
);
}
Expand Down
66 changes: 0 additions & 66 deletions src/components/RoomNameInput/index.native.js
kowczarz marked this conversation as resolved.
Outdated
Show resolved Hide resolved

This file was deleted.

9 changes: 6 additions & 3 deletions src/components/RoomNameInput/roomNameInputPropTypes.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import PropTypes from 'prop-types';
import refPropTypes from '@components/refPropTypes';

const propTypes = {
/** Callback to execute when the text input is modified correctly */
Expand All @@ -14,10 +15,10 @@ const propTypes = {
errorText: PropTypes.oneOfType([PropTypes.string, PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.object]))]),

/** A ref forwarded to the TextInput */
forwardedRef: PropTypes.func,
forwardedRef: refPropTypes,

/** The ID used to uniquely identify the input in a Form */
inputID: PropTypes.string,
inputID: PropTypes.string.isRequired,

/** Callback that is called when the text input is blurred */
onBlur: PropTypes.func,
Expand All @@ -30,6 +31,8 @@ const propTypes = {

/** Whether navigation is focused */
isFocused: PropTypes.bool.isRequired,

roomName: PropTypes.string,
};

const defaultProps = {
Expand All @@ -39,10 +42,10 @@ const defaultProps = {
errorText: '',
forwardedRef: () => {},

inputID: undefined,
onBlur: () => {},
autoFocus: false,
shouldDelayFocus: false,
roomName: '',
};

export {propTypes, defaultProps};
4 changes: 2 additions & 2 deletions src/libs/RoomNameInputUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ import CONST from '@src/CONST';
/**
* Replaces spaces with dashes
*/
function modifyRoomName(roomName: string): string {
function modifyRoomName(roomName: string, skipPolicyPrefix?: boolean): string {
const modifiedRoomNameWithoutHash = roomName
.replace(/ /g, '-')

// Replaces the smart dash on iOS devices with two hyphens
.replace(/—/g, '--');

return `${CONST.POLICY.ROOM_PREFIX}${modifiedRoomNameWithoutHash}`;
return skipPolicyPrefix ? modifiedRoomNameWithoutHash : `${CONST.POLICY.ROOM_PREFIX}${modifiedRoomNameWithoutHash}`;
}

export {
Expand Down
16 changes: 6 additions & 10 deletions src/pages/settings/Report/RoomNamePage.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import React, {useCallback, useRef} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import FullPageNotFoundView from '@components/BlockingViews/FullPageNotFoundView';
import Form from '@components/Form';
import FormProvider from '@components/Form/FormProvider';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import RoomNameInput from '@components/RoomNameInput';
import ScreenWrapper from '@components/ScreenWrapper';
Expand Down Expand Up @@ -42,13 +42,8 @@ const defaultProps = {
policy: {},
};

function RoomNamePage(props) {
function RoomNamePage({policy, report, reports, translate}) {
const styles = useThemeStyles();
const policy = props.policy;
const report = props.report;
const reports = props.reports;
const translate = props.translate;

const roomNameInputRef = useRef(null);
const isFocused = useIsFocused();

Expand Down Expand Up @@ -91,7 +86,7 @@ function RoomNamePage(props) {
title={translate('newRoomPage.roomName')}
onBackButtonPress={() => Navigation.goBack(ROUTES.REPORT_SETTINGS.getRoute(report.reportID))}
/>
<Form
<FormProvider
style={[styles.flexGrow1, styles.ph5]}
formID={ONYXKEYS.FORMS.ROOM_NAME_FORM}
onSubmit={(values) => Report.updatePolicyRoomNameAndNavigate(report, values.roomName)}
Expand All @@ -101,13 +96,14 @@ function RoomNamePage(props) {
>
<View style={styles.mb4}>
<RoomNameInput
ref={(ref) => (roomNameInputRef.current = ref)}
ref={roomNameInputRef}
inputID="roomName"
defaultValue={report.reportName}
isFocused={isFocused}
roomName={report.reportName.slice(1)}
/>
</View>
</Form>
</FormProvider>
</FullPageNotFoundView>
</ScreenWrapper>
);
Expand Down
Loading
Loading