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

OptionsSelector display max length error with counter when exceeded #28722

Merged
merged 12 commits into from
Oct 11, 2023
Merged

OptionsSelector display max length error with counter when exceeded #28722

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
05095d0
OptionsSelector display max length error with counter when exceeded
ahmedGaber93 Oct 2, 2023
64d8a3d
Merge branch 'main' into issue-25804
ahmedGaber93 Oct 3, 2023
40354e1
update translations
ahmedGaber93 Oct 3, 2023
7728993
prettier
ahmedGaber93 Oct 3, 2023
fde11f9
OptionsSelector display max length error with counter when exceeded
ahmedGaber93 Oct 5, 2023
afc7d69
OptionsSelector display max length error with counter when exceeded
ahmedGaber93 Oct 5, 2023
9b98e98
update en.ts file
ahmedGaber93 Oct 6, 2023
731219c
Merge branch 'main' into issue-25804
ahmedGaber93 Oct 9, 2023
51e0e9c
Merge branch 'main' into issue-25804
ahmedGaber93 Oct 10, 2023
0c00e23
add comment for CONST.ERROR_EXCEED_RANGE
ahmedGaber93 Oct 10, 2023
b57df36
Merge branch 'main' into issue-25804
ahmedGaber93 Oct 10, 2023
d5e39c2
rename CONST.ERROR_EXCEED_RANGE
ahmedGaber93 Oct 11, 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
2 changes: 2 additions & 0 deletions src/CONST.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2684,6 +2684,8 @@ const CONST = {
HIDDEN_MARGIN_VERTICAL: 0,
HIDDEN_BORDER_BOTTOM_WIDTH: 0,
},
SEARCH_MAX_LENGTH: 500,
ERROR_EXCEED_RANGE: 20,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ERROR_EXCEED_RANGE: 20,
/**
* The count of characters we'll allow the user to type after reaching SEARCH_MAX_LENGTH in an input.
*/
ADDITIONAL_ALLOWED_CHARACTERS: 20,

Thanks! Now that I understand what this is, I can suggest what I think is a better name along with some nice docs.

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, the comment makes it more understandable.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about renaming the variable itself?

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, correct, I missed that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flodnv updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

} as const;

export default CONST;
17 changes: 14 additions & 3 deletions src/components/OptionsSelector/BaseOptionsSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class BaseOptionsSelector extends Component {
this.selectRow = this.selectRow.bind(this);
this.selectFocusedOption = this.selectFocusedOption.bind(this);
this.addToSelection = this.addToSelection.bind(this);
this.updateSearchValue = this.updateSearchValue.bind(this);
this.relatedTarget = null;

const allOptions = this.flattenSections();
Expand All @@ -63,6 +64,7 @@ class BaseOptionsSelector extends Component {
allOptions,
focusedIndex,
shouldDisableRowSelection: false,
errorMessage: '',
};
}

Expand Down Expand Up @@ -166,6 +168,14 @@ class BaseOptionsSelector extends Component {
return defaultIndex;
}

updateSearchValue(value) {
this.setState({
errorMessage: value.length > this.props.maxLength ? this.props.translate('common.error.characterLimitExceedCounter', {length: value.length, limit: this.props.maxLength}) : '',
});

this.props.onChangeText(value);
}

subscribeToKeyboardShortcut() {
const enterConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;
this.unsubscribeEnter = KeyboardShortcut.subscribe(
Expand Down Expand Up @@ -365,10 +375,11 @@ class BaseOptionsSelector extends Component {
label={this.props.textInputLabel}
accessibilityLabel={this.props.textInputLabel}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.TEXT}
onChangeText={this.props.onChangeText}
onChangeText={this.updateSearchValue}
errorText={this.state.errorMessage}
onSubmitEditing={this.selectFocusedOption}
placeholder={this.props.placeholderText}
maxLength={this.props.maxLength}
maxLength={this.props.maxLength + CONST.ERROR_EXCEED_RANGE}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused, can you explain this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flodnv
We decide to use the below design.
We will allow the user to type more than the maxLength, but we will display an error with counter like the image below, and adding upper bound that stops the input.
So the input maxLength should be maxLength + the range we will display the exceeded counter
E.g. maxLength = 500 + 20 will show the error from 501 to 520

265538293-0cb24617-c838-48cd-b314-f94dbcaf5755

Copy link
Contributor

Choose a reason for hiding this comment

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

E.g. maxLength = 500 + 20 will show the error from 501 to 520

Sorry I am not seeing what that means on your screenshot 😕 What do you mean by "will show the error from 501 to 520"? What and where will the "501 to 520" error show? 😕

Copy link
Contributor Author

@ahmedGaber93 ahmedGaber93 Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@flodnv
Sorry about wrong screenshot, this is the correct screenshot.
Screenshot 2023-10-10 at 2 59 22 PM

// the length can user type it without see the error with counter in the scrrenshot above.
this.props.maxLength = 500;

// the range we will display the error from 501 to 520
CONST.ERROR_EXCEED_RANGE = 20;

// so the max length that user can type at all and the input will allow it will be
maxLength={this.props.maxLength + CONST.ERROR_EXCEED_RANGE}

501 to 520 mean when the typed text length is between 501 and 520 characters, the error with counter will be displayed.

Copy link
Contributor

Choose a reason for hiding this comment

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

So if I understand this correctly, it is impossible to type past 520 chars? What if I paste 1000 chars there?

Copy link
Contributor Author

@ahmedGaber93 ahmedGaber93 Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we paste 1000 char, it will paste only 520 char, because the input maxLength don't let it accept more than 520 char, and this is the default behavior for maxLength, you can try here

keyboardType={this.props.keyboardType}
onBlur={(e) => {
if (!this.props.shouldFocusOnSelectRow) {
Expand All @@ -394,7 +405,7 @@ class BaseOptionsSelector extends Component {
multipleOptionSelectorButtonText={this.props.multipleOptionSelectorButtonText}
onAddToSelection={this.addToSelection}
hideSectionHeaders={this.props.hideSectionHeaders}
headerMessage={this.props.headerMessage}
headerMessage={this.state.errorMessage ? '' : this.props.headerMessage}
boldStyle={this.props.boldStyle}
showTitleTooltip={this.props.showTitleTooltip}
isDisabled={this.props.isDisabled}
Expand Down
3 changes: 2 additions & 1 deletion src/components/OptionsSelector/optionsSelectorPropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import PropTypes from 'prop-types';
import optionPropTypes from '../optionPropTypes';
import styles from '../../styles/styles';
import CONST from '../../CONST';

const propTypes = {
/** Callback to fire when a row is tapped */
Expand Down Expand Up @@ -154,7 +155,7 @@ const defaultProps = {
isDisabled: false,
shouldHaveOptionSeparator: false,
initiallyFocusedOptionKey: undefined,
maxLength: undefined,
maxLength: CONST.SEARCH_MAX_LENGTH,
shouldShowTextInput: true,
onChangeText: () => {},
shouldUseStyleForChildren: true,
Expand Down
1 change: 1 addition & 0 deletions src/languages/en.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ export default {
phoneNumber: `Please enter a valid phone number, with the country code (e.g. ${CONST.EXAMPLE_PHONE_NUMBER})`,
fieldRequired: 'This field is required.',
characterLimit: ({limit}: CharacterLimitParams) => `Exceeds the maximum length of ${limit} characters`,
characterLimitExceedCounter: ({length, limit}) => `Character limit exceeded (${length}/${limit})`,
dateInvalid: 'Please select a valid date',
invalidCharacter: 'Invalid character',
enterMerchant: 'Enter a merchant name',
Expand Down
1 change: 1 addition & 0 deletions src/languages/es.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ export default {
phoneNumber: `Introduce un teléfono válido, incluyendo el código del país (p. ej. ${CONST.EXAMPLE_PHONE_NUMBER})`,
fieldRequired: 'Este campo es obligatorio.',
characterLimit: ({limit}: CharacterLimitParams) => `Supera el límite de ${limit} caracteres`,
characterLimitExceedCounter: ({length, limit}) => `Se superó el límite de caracteres (${length}/${limit})`,
dateInvalid: 'Por favor, selecciona una fecha válida',
invalidCharacter: 'Carácter invalido',
enterMerchant: 'Introduce un comerciante',
Expand Down