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 #29771

Merged
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
5248bc6
Form validation fixes
kowczarz Oct 5, 2023
5b4a419
Form docs improvements
kowczarz Oct 6, 2023
bd87379
Fix prop types
kowczarz Oct 6, 2023
10ea700
Remove console log
kowczarz Oct 6, 2023
eab18e6
Refactor roomNameInput
kowczarz Oct 12, 2023
40a6da2
Refactor RoomNamePage
kowczarz Oct 12, 2023
e35bddc
Refactor WorkspaceNewRoomPage
kowczarz Oct 12, 2023
2e12b63
Merge remote-tracking branch 'expensify/main' into from-migration/roo…
kowczarz Oct 16, 2023
1fd92cc
Post-merge fixes
kowczarz Oct 16, 2023
ba1a78d
Prettier fixes
kowczarz Oct 16, 2023
fefdb2b
Add value parser to the new form
kowczarz Oct 17, 2023
e05b537
Remove native room name input
kowczarz Oct 17, 2023
eff3e88
Update room name input
kowczarz Oct 17, 2023
187fa7d
Merge remote-tracking branch 'expensify/main' into from-migration/roo…
kowczarz Oct 17, 2023
398b842
Prettier
kowczarz Oct 17, 2023
edd40b9
Merge remote-tracking branch 'expensify/main' into from-migration/roo…
kowczarz Oct 18, 2023
bd686b2
Code review changes
kowczarz Oct 18, 2023
d68434e
Merge remote-tracking branch 'expensify/main' into from-migration/roo…
kowczarz Oct 24, 2023
a37be42
Merge remote-tracking branch 'expensify/main' into from-migration/roo…
kowczarz Oct 25, 2023
6edeecf
Remove empty file
kowczarz Oct 25, 2023
f0b7aea
Fix wrong ref
kowczarz Oct 26, 2023
565ea7c
Merge remote-tracking branch 'expensify/main' into from-migration/roo…
kowczarz Oct 26, 2023
19077c1
Fix wrong ref passing
kowczarz Oct 26, 2023
ccf0b9f
Fix propType
kowczarz Oct 26, 2023
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
24 changes: 16 additions & 8 deletions src/components/Form/FormProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,13 +227,15 @@ function FormProvider({validate, formID, shouldValidateOnBlur, shouldValidateOnC
.first()
.value() || '';

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

return {
...propsToParse,
ref: newRef,
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 @@ -273,13 +275,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`]: inputValue,
}
: {
...prevState,
[inputKey]: inputValue,
};

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

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

if (_.isFunction(propsToParse.onValueChange)) {
propsToParse.onValueChange(value, inputKey);
propsToParse.onValueChange(inputValue, inputKey);
}
},
};
Expand Down
2 changes: 2 additions & 0 deletions src/components/Form/InputWrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ const propTypes = {
inputID: PropTypes.string.isRequired,
valueType: PropTypes.string,
forwardedRef: PropTypes.oneOfType([PropTypes.func, PropTypes.shape({current: PropTypes.instanceOf(React.Component)})]),
valueParser: PropTypes.func,
};

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

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

function RoomNameInput({isFocused, autoFocus, disabled, errorText, forwardedRef, value, onBlur, onChangeText, onInputChange, shouldDelayFocus}) {
function RoomNameInput({isFocused, autoFocus, disabled, errorText, forwardedRef, onBlur, shouldDelayFocus, inputID}) {
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 || '';
Comment on lines -28 to -32
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer an issue with the new solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, with current approach if we are parsing the value of the input, it's completely separated from the value we display, so it's faster (we are limiting numbers of re-renders) and it doesn't affect the cursor.

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 = (roomName) => RoomNameInputUtils.modifyRoomName(roomName);

return (
<TextInput
<InputWrapper
InputComponent={TextInput}
inputID={inputID}
ref={forwardedRef}
disabled={disabled}
label={translate('newRoomPage.roomName')}
accessibilityLabel={translate('newRoomPage.roomName')}
accessibilityRole={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}
autoCapitalize="none"
onBlur={() => isFocused && onBlur()}
shouldDelayFocus={shouldDelayFocus}
autoFocus={isFocused && autoFocus}
maxLength={CONST.REPORT.MAX_ROOM_NAME_LENGTH}
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
62 changes: 0 additions & 62 deletions src/components/RoomNameInput/index.native.js

This file was deleted.

3 changes: 1 addition & 2 deletions src/components/RoomNameInput/roomNameInputPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const propTypes = {
forwardedRef: PropTypes.func,

/** 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 @@ -39,7 +39,6 @@ const defaultProps = {
errorText: '',
forwardedRef: () => {},

inputID: undefined,
onBlur: () => {},
autoFocus: false,
shouldDelayFocus: false,
Expand Down
6 changes: 3 additions & 3 deletions src/pages/settings/Report/RoomNamePage.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import CONST from '../../../CONST';
import ScreenWrapper from '../../../components/ScreenWrapper';
import HeaderWithBackButton from '../../../components/HeaderWithBackButton';
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';
import Form from '../../../components/Form';
import ONYXKEYS from '../../../ONYXKEYS';
import styles from '../../../styles/styles';
import Navigation from '../../../libs/Navigation/Navigation';
Expand All @@ -21,6 +20,7 @@ import * as Report from '../../../libs/actions/Report';
import RoomNameInput from '../../../components/RoomNameInput';
import * as ReportUtils from '../../../libs/ReportUtils';
import FullPageNotFoundView from '../../../components/BlockingViews/FullPageNotFoundView';
import FormProvider from '../../../components/Form/FormProvider';

const propTypes = {
...withLocalizePropTypes,
Expand Down Expand Up @@ -90,7 +90,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 @@ -106,7 +106,7 @@ function RoomNamePage(props) {
isFocused={isFocused}
/>
</View>
</Form>
</FormProvider>
</FullPageNotFoundView>
</ScreenWrapper>
);
Expand Down
19 changes: 12 additions & 7 deletions src/pages/workspace/WorkspaceNewRoomPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,14 @@ import * as ErrorUtils from '../../libs/ErrorUtils';
import * as ValidationUtils from '../../libs/ValidationUtils';
import * as ReportUtils from '../../libs/ReportUtils';
import * as PolicyUtils from '../../libs/PolicyUtils';
import Form from '../../components/Form';
import policyMemberPropType from '../policyMemberPropType';
import FullPageNotFoundView from '../../components/BlockingViews/FullPageNotFoundView';
import compose from '../../libs/compose';
import variables from '../../styles/variables';
import useDelayedInputFocus from '../../hooks/useDelayedInputFocus';
import ValuePicker from '../../components/ValuePicker';
import FormProvider from '../../components/Form/FormProvider';
import InputWrapper from '../../components/Form/InputWrapper';

const propTypes = {
/** All reports shared with the user */
Expand Down Expand Up @@ -182,7 +183,7 @@ function WorkspaceNewRoomPage(props) {
// This is because when wrapping whole screen the screen was freezing when changing Tabs.
keyboardVerticalOffset={variables.contentHeaderHeight + variables.tabSelectorButtonHeight + variables.tabSelectorButtonPadding + insets.top}
>
<Form
<FormProvider
formID={ONYXKEYS.FORMS.NEW_ROOM_FORM}
submitButtonText={translate('newRoomPage.createRoom')}
style={[styles.mh5, styles.flexGrow1]}
Expand All @@ -200,7 +201,8 @@ function WorkspaceNewRoomPage(props) {
/>
</View>
<View style={styles.mb5}>
<TextInput
<InputWrapper
InputComponent={TextInput}
inputID="welcomeMessage"
label={translate('welcomeMessagePage.welcomeMessageOptional')}
accessibilityLabel={translate('welcomeMessagePage.welcomeMessageOptional')}
Expand All @@ -213,7 +215,8 @@ function WorkspaceNewRoomPage(props) {
/>
</View>
<View style={[styles.mhn5]}>
<ValuePicker
<InputWrapper
InputComponent={ValuePicker}
inputID="policyID"
label={translate('workspace.common.workspace')}
placeholder={translate('newRoomPage.selectAWorkspace')}
Expand All @@ -223,7 +226,8 @@ function WorkspaceNewRoomPage(props) {
</View>
{isPolicyAdmin && (
<View style={styles.mhn5}>
<ValuePicker
<InputWrapper
InputComponent={ValuePicker}
inputID="writeCapability"
label={translate('writeCapabilityPage.label')}
items={writeCapabilityOptions}
Expand All @@ -233,7 +237,8 @@ function WorkspaceNewRoomPage(props) {
</View>
)}
<View style={[styles.mb1, styles.mhn5]}>
<ValuePicker
<InputWrapper
InputComponent={ValuePicker}
inputID="visibility"
label={translate('newRoomPage.visibility')}
items={visibilityOptions}
Expand All @@ -242,7 +247,7 @@ function WorkspaceNewRoomPage(props) {
/>
</View>
<Text style={[styles.textLabel, styles.colorMuted]}>{visibilityDescription}</Text>
</Form>
</FormProvider>
</KeyboardAvoidingView>
)}
</ScreenWrapper>
Expand Down
Loading