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

Surface room name errors and warn users about reserved policy room name error #7227

Merged
merged 18 commits into from
Jan 14, 2022
Merged

Surface room name errors and warn users about reserved policy room name error #7227

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
d895122
Warn users about reserved policy room name error
Jan 14, 2022
3c9c70f
lint fix error
Jan 14, 2022
73b0628
Make error logic cleaner
Jan 14, 2022
deea616
Comment for reserved names error
Jan 14, 2022
7abda6b
Put reserved names in const
Jan 14, 2022
c5fbf13
Add spanish trasnlation
Jan 14, 2022
89830a5
Add inline error to room name input
Jan 14, 2022
e9c5e02
Merge branch 'main' of github.com:Expensify/App into amal-ucpr-reserv…
Jan 14, 2022
6dd5cc2
Refer to as errors
Jan 14, 2022
006a6d5
Better comment
Jan 14, 2022
8d3cd4e
Move input to TextInputWithPrefix
Jan 14, 2022
744bb1c
Use original TextInput from react for this component to be compatible…
Jan 14, 2022
8275217
lint unused import
Jan 14, 2022
950a2a0
Add InlineError to TextInputWithPrefix
Jan 14, 2022
a560e0c
spacing typo fix
Jan 14, 2022
afb5743
Remove TextLabelWithInput and simplify TextInputWithPrefix
Jan 14, 2022
3ee7b69
Add missing import
Jan 14, 2022
fa6bc81
remove extra newline
Jan 14, 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
1 change: 1 addition & 0 deletions src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ const CONST = {
RESTRICTED: 'restricted',
PRIVATE: 'private',
},
RESERVED_ROOM_NAMES: ['#admins', '#announce'],
MAX_PREVIEW_AVATARS: 4,
MAX_ROOM_NAME_LENGTH: 80,
},
Expand Down
44 changes: 29 additions & 15 deletions src/components/RoomNameInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import styles from '../styles/styles';
import compose from '../libs/compose';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import withFullPolicy, {fullPolicyDefaultProps, fullPolicyPropTypes} from '../pages/workspace/withFullPolicy';

import TextInputWithPrefix from './TextInputWithPrefix';
import InlineErrorText from './InlineErrorText';

const propTypes = {
/** Callback to execute when the text input is modified correctly */
Expand Down Expand Up @@ -105,11 +105,18 @@ class RoomNameInput extends Component {
report => report && report.policyID === this.props.policyID && report.reportName === finalRoomName,
);

let error = '';

// We error if the room name already exists. We don't care if it matches the original name provided in this
// component because then we are not changing the room's name.
const error = isExistingRoomName && finalRoomName !== this.originalRoomName
? this.props.translate('newRoomPage.roomAlreadyExists')
: '';
if (isExistingRoomName && finalRoomName !== this.originalRoomName) {
error = this.props.translate('newRoomPage.roomAlreadyExistsError');
}

// Certain names are reserved for default rooms and should not be used for policy rooms.
if (_.contains(CONST.REPORT.RESERVED_ROOM_NAMES, finalRoomName)) {
error = this.props.translate('newRoomPage.roomNameReservedError');
}

this.setState({
roomName: finalRoomName,
Expand All @@ -119,17 +126,24 @@ class RoomNameInput extends Component {

render() {
return (
<TextInputWithPrefix
disabled={this.props.disabled}
label={this.props.translate('newRoomPage.roomName')}
prefixCharacter="#"
placeholder={this.props.translate('newRoomPage.social')}
containerStyles={[styles.mb5]}
onChangeText={roomName => this.checkAndModifyRoomName(roomName)}
value={this.state.roomName.substring(1)}
errorText={this.state.error}
autoCapitalize="none"
/>
<>
<TextInputWithPrefix
disabled={this.props.disabled}
label={this.props.translate('newRoomPage.roomName')}
prefixCharacter="#"
placeholder={this.props.translate('newRoomPage.social')}
containerStyles={[styles.mb5]}
onChangeText={roomName => this.checkAndModifyRoomName(roomName)}
value={this.state.roomName.substring(1)}
errorText={this.state.error}
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB since the input functionality is unrelated to these changes. It's really weird that we pass errorText here, but then omit passing it on to TextInput and have to use InlineErrorText to display the error. Not sure why this is the case @jasperhuangg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OH good catch! Honestly maybe it's better to just pass it in? Maybe there was a weird styling reason why we didn't do it. Let me try modifying that omit and seeing if that makes it better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given how TextInputWithPrefix currently works, we will have to pass it here so the outline border changes to red. I'm not sure why we omit it though, but I assume it's intentional/necessary since it was added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm removing the omit doesn't work - no error text shows up. Maybe it has to do with us using the base ReactNative TextInput there in TextInputWithPrefix/index.js and it doesn't seem to use the errorText correctly. I think I'll still move around the error to TextInputWithPrefix and out of TextInputWithLabel (I don't think this is used anymore)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok I know why we don't use the BaseTextInput we created. It's a less configurable input. I think it still makes sense to move the error around to the TextInputWithPrefix though, so I'll try to do that.
image

autoCapitalize="none"
/>
{!_.isEmpty(this.state.error) && (
<InlineErrorText>
{this.state.error}
</InlineErrorText>
)}
</>
);
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/languages/en.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,8 @@ export default {
restrictedDescription: 'People in your workspace are able to find this room using Search',
privateDescription: 'Only people invited to this room are able to find it',
createRoom: 'Create Room',
roomAlreadyExists: 'A room with this name already exists',
roomAlreadyExistsError: 'A room with this name already exists',
roomNameReservedError: 'This name is reserved and cannot be used',
social: 'social',
selectAWorkspace: 'Select a workspace',
growlMessageOnError: 'Unable to create policy room, please check your connection and try again.',
Expand Down
3 changes: 2 additions & 1 deletion src/languages/es.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,7 +876,8 @@ export default {
restrictedDescription: 'Sólo las personas en tu espacio de trabajo pueden encontrar esta sala a través de "Buscar"',
privateDescription: 'Sólo las personas que están invitadas a esta sala pueden encontrarla',
createRoom: 'Crea una sala de chat',
roomAlreadyExists: 'Ya existe una sala con este nombre',
roomAlreadyExistsError: 'Ya existe una sala con este nombre',
roomNameReservedError: 'Este nombre está reservado y no puede usarse',
social: 'social',
selectAWorkspace: 'Seleccionar un espacio de trabajo',
growlMessageOnError: 'No ha sido posible crear el espacio de trabajo, por favor comprueba tu conexión e inténtalo de nuevo.',
Expand Down