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

feat(search): Debounce and phone number validation #3124

Closed
wants to merge 11 commits into from
Closed

feat(search): Debounce and phone number validation #3124

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
74f0c59
feat: Debouncing of search with phone number validation
pranshuchittora May 25, 2021
f1ee5ff
feat: Loader on search page
pranshuchittora May 18, 2021
f9f000a
feat: Design changes
pranshuchittora May 20, 2021
3effa64
feat: No email message
pranshuchittora May 25, 2021
cf25b91
feat: Err msg or results not both
pranshuchittora Jun 6, 2021
2000719
Merge remote-tracking branch 'origin/main' into pranshuchittora/fix/2936
pranshuchittora Jun 7, 2021
8a8b1cf
feat: Liniting changes
pranshuchittora Jun 7, 2021
4c41ecc
fix: Minor linitng changes
pranshuchittora Jun 7, 2021
39581a4
fix: Minor styling fixes
pranshuchittora Jun 9, 2021
429d9aa
Merge remote-tracking branch 'origin/main' into pranshuchittora/fix/2936
pranshuchittora Jun 9, 2021
cfb09ce
feat: PR review changes
pranshuchittora Jun 11, 2021
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
23 changes: 16 additions & 7 deletions src/components/OptionsList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ const propTypes = {
/** Optional header message */
headerMessage: PropTypes.string,

/** Sets min height of header message container */
headerMessageMinHeight: PropTypes.oneOf([PropTypes.number, PropTypes.string]),

/** Passed via forwardRef so we can access the SectionList ref */
innerRef: PropTypes.oneOfType([
PropTypes.func,
Expand All @@ -74,6 +77,7 @@ const propTypes = {

/** Toggle between compact and default view of the option */
optionMode: PropTypes.oneOf(['compact', 'default']),

};

const defaultProps = {
Expand All @@ -91,9 +95,11 @@ const defaultProps = {
forceTextUnreadStyle: false,
onSelectRow: () => {},
headerMessage: '',
headerMessageMinHeight: 0,
innerRef: null,
showTitleTooltip: false,
optionMode: undefined,

};

class OptionsList extends Component {
Expand Down Expand Up @@ -201,13 +207,16 @@ class OptionsList extends Component {
render() {
return (
<View style={this.props.listContainerStyles}>
{this.props.headerMessage ? (
<View style={[styles.ph5, styles.pb5]}>
<Text style={[styles.textLabel, styles.colorMuted]}>
{this.props.headerMessage}
</Text>
</View>
) : null}
<View style={[{minHeight: this.props.headerMessageMinHeight}]}>
{this.props.headerMessage ? (
<View style={[styles.ph5]}>
<Text style={[styles.textLabel, styles.colorMuted]}>
{this.props.headerMessage}
</Text>
</View>
) : null}

pranshuchittora marked this conversation as resolved.
Show resolved Hide resolved
</View>
<SectionList
ref={this.props.innerRef}
bounces={false}
Expand Down
15 changes: 13 additions & 2 deletions src/components/OptionsSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,19 @@ class OptionsSelector extends Component {
render() {
return (
<View style={[styles.flex1]}>
<View style={[styles.ph5, styles.pv3]}>
<View style={[
styles.ph5,
styles.pt3,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think none of the changes in this file are needed, the only difference I can see that your changes make is they reduce the padding below the following TextInputWithFocusStyles, but I think it looks find with the original padding. Is there any other change you were trying to make with these style updates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverting back

styles.pb1,
styles.dFlex,
styles.flexRow,
styles.alignItemsCenter,
]}
>
<TextInputWithFocusStyles
styleFocusIn={[styles.textInputReversedFocus]}
ref={el => this.textInput = el}
style={[styles.textInput]}
style={[styles.textInput, styles.flex1]}
value={this.props.value}
onChangeText={this.props.onChangeText}
onKeyPress={this.handleKeyPress}
Expand All @@ -203,6 +211,7 @@ class OptionsSelector extends Component {
placeholderTextColor={themeColors.placeholderText}
/>
</View>

<OptionsList
ref={el => this.list = el}
optionHoveredStyle={styles.hoveredComponentBG}
Expand All @@ -213,11 +222,13 @@ class OptionsSelector extends Component {
canSelectMultipleOptions={this.props.canSelectMultipleOptions}
hideSectionHeaders={this.props.hideSectionHeaders}
headerMessage={this.props.headerMessage}
headerMessageMinHeight={40}
disableFocusOptions={this.props.disableArrowKeysActions}
hideAdditionalOptionStates={this.props.hideAdditionalOptionStates}
forceTextUnreadStyle={this.props.forceTextUnreadStyle}
showTitleTooltip={this.props.showTitleTooltip}
/>

</View>
);
}
Expand Down
1 change: 1 addition & 0 deletions src/languages/en.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ export default {
cameraPermissionsNotGranted: 'Camera permissions not granted',
messages: {
noPhoneNumber: 'Please enter a phone number including the country code e.g +447814266907',
noEmailOrPhone: 'Please enter a valid email address or phone number.',
maxParticipantsReached: 'You\'ve reached the maximum number of participants for a group chat.',
},
session: {
Expand Down
14 changes: 14 additions & 0 deletions src/libs/API.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,19 @@ function GetCurrencyList() {
return Mobile_GetConstants({data: ['currencyList']});
}

/**
* Validates PhoneNumber
*
* @param {Object} parameters
* @param {String} parameters.phoneNumber
* @returns {Promise}
*/
function IsValidPhoneNumber(parameters) {
const commandName = 'IsValidPhoneNumber';
requireParameters(['phoneNumber'], parameters, commandName);
return Network.post(commandName, parameters);
}

export {
Authenticate,
BankAccount_Create,
Expand All @@ -798,6 +811,7 @@ export {
GetIOUReport,
GetRequestCountryCode,
Graphite_Timer,
IsValidPhoneNumber,
Log,
PayIOU,
PersonalDetails_GetForEmails,
Expand Down
161 changes: 114 additions & 47 deletions src/pages/SearchPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, {Component} from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'lodash';
import {withOnyx} from 'react-native-onyx';
import OptionsSelector from '../components/OptionsSelector';
import {getSearchOptions, getHeaderMessage} from '../libs/OptionsListUtils';
Expand All @@ -9,14 +10,17 @@ import styles from '../styles/styles';
import KeyboardSpacer from '../components/KeyboardSpacer';
import Navigation from '../libs/Navigation/Navigation';
import ROUTES from '../ROUTES';
import withWindowDimensions, {windowDimensionsPropTypes} from '../components/withWindowDimensions';
import withWindowDimensions, {
windowDimensionsPropTypes,
} from '../components/withWindowDimensions';
import {fetchOrCreateChatReport} from '../libs/actions/Report';
import HeaderWithCloseButton from '../components/HeaderWithCloseButton';
import ScreenWrapper from '../components/ScreenWrapper';
import Timing from '../libs/actions/Timing';
import CONST from '../CONST';
import withLocalize, {withLocalizePropTypes} from '../components/withLocalize';
import compose from '../libs/compose';
import * as API from '../libs/API';

const personalDetailsPropTypes = PropTypes.shape({
/** The login of the person (either email or phone number) */
Expand Down Expand Up @@ -47,6 +51,9 @@ const propTypes = {
email: PropTypes.string.isRequired,
}).isRequired,

/** */
countryCode: PropTypes.string.isRequired,

/** Window Dimensions Props */
...windowDimensionsPropTypes,

Expand All @@ -60,19 +67,18 @@ class SearchPage extends Component {
Timing.start(CONST.TIMING.SEARCH_RENDER);

this.selectReport = this.selectReport.bind(this);

const {
recentReports,
personalDetails,
userToInvite,
} = getSearchOptions(
this.validateInput = _.debounce(this.validateInput.bind(this), 300);
const {recentReports, personalDetails, userToInvite} = getSearchOptions(
Copy link
Contributor

Choose a reason for hiding this comment

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

This change isn't necessary - please try to keep lines the same as before, unless you're changing something in the line(s).

props.reports,
props.personalDetails,
'',
);

this.preserveRecentReports = recentReports;

this.state = {
searchValue: '',
headerMessage: '',
recentReports,
personalDetails,
userToInvite,
Expand All @@ -84,10 +90,10 @@ class SearchPage extends Component {
}

/**
* Returns the sections needed for the OptionsSelector
*
* @returns {Array}
*/
* Returns the sections needed for the OptionsSelector
*
* @returns {Array}
*/
getSections() {
const sections = [{
title: this.props.translate('common.recents'),
Expand All @@ -97,48 +103,104 @@ class SearchPage extends Component {
}];

if (this.state.userToInvite) {
sections.push(({
sections.push({
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
undefined,
data: [this.state.userToInvite],
shouldShow: true,
indexOffset: 0,
}));
});
}

return sections;
}

/**
* Reset the search value and redirect to the selected report
*
* @param {Object} option
*/
* Reset the search value and redirect to the selected report
*
* @param {Object} option
*/
pranshuchittora marked this conversation as resolved.
Show resolved Hide resolved
selectReport(option) {
if (!option) {
return;
}

if (option.reportID) {
this.setState({
searchValue: '',
}, () => {
Navigation.navigate(ROUTES.getReportRoute(option.reportID));
});
this.setState(
{
searchValue: '',
},
() => {
Navigation.navigate(ROUTES.getReportRoute(option.reportID));
},
);
} else {
fetchOrCreateChatReport([
this.props.session.email,
option.login,
]);
fetchOrCreateChatReport([this.props.session.email, option.login]);
}
}

render() {
const sections = this.getSections();
validateInput(searchValue) {
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we're going to have to move this into OptionsListUtils since the validation you're adding is going to apply to all pages that have account-search functionality. Can you figure out how we can reuse getSearchOptions here (by editing getOptions)? I'm thinking we can have getOptions also return headerMessage

Copy link
Contributor Author

@pranshuchittora pranshuchittora Jun 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I discussed this with you earlier. That my initial approach was to add the changes in getOptions only but it's not flexible enough and might require good amount of refactoring.

Because there are a lot of things happening internally and the code is a bit messy and adding more code will increase the complexity and maintainability efforts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Woah you did? I'm sorry I don't remember you bringing up that approach, maybe I misunderstood :O

Hmm I don't see why we can't refactor getHeaderMessage just a bit, and make getOptions also return something like headerMessage: getMeaderMessage(...). Here's why I'm thinking this will work (please let me know if you disagree on these points):

  1. getHeaderMessage takes only a few parameters, and all of the params should be able to be calculated from other variables inside getOptions
  2. getOptions is only used in OptionsListUtils, which has get...Options functions for specific pages, all of those pages will need this type of functionality (the same type of logic for headerMessage)

Copy link
Contributor Author

@pranshuchittora pranshuchittora Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @Beamanator
I tried merging these changes into utils. So getSearchOptions & getHeaderMessage are independent of each other. They both take the searchValue as one of the input. With this, we might end up hitting the phone number validation API twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm once you push your latest changes I can check and see if I can help come up with a way to only hit that validation API once 👍

if (!searchValue) {
return;
}

let modifiedSearchValue = searchValue;
const headerMessage = getHeaderMessage(
(this.state.recentReports.length + this.state.personalDetails.length) !== 0,
Boolean(this.state.userToInvite),
this.state.searchValue,
this.state.personalDetails.length !== 0,
false,
searchValue,
);

if (/^\d+$/.test(searchValue) || /^[0-9]*$/.test(searchValue)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe ^[0-9]*$ matches even an empty string, is that known / desired? Can you combine these two into the same expression?

I also believe \d is the same as [0-9] so if you can't combine them, at least we can be consistent with \d or [0-9]

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this doesn't correctly check if a typed phone number already has a + in front. I believe we can use Str.isValidPhone instead of coming up with a new regexp here right?

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 tried using Str.isValidPhone but it is not working properly. Also I am using || so that if any of the regex matches then it skips the checks for second one. Combining them is possible but will increase the time complexity of the program.

// Appends country code
if (!searchValue.includes('+')) {
modifiedSearchValue = `+${this.props.countryCode}${searchValue}`;
}
API.IsValidPhoneNumber({phoneNumber: modifiedSearchValue}).then(
(resp) => {
if (resp.isValid) {
const {
recentReports,
personalDetails,
userToInvite,
} = getSearchOptions(
this.props.reports,
this.props.personalDetails,
searchValue,
);
this.setState({
userToInvite,
recentReports,
personalDetails,
headerMessage: '',
});
} else {
this.setState({
recentReports: this.preserveRecentReports,
userToInvite: null,
headerMessage,
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, let's do something like headerMessage: translate(preferredLocale, 'messages.noPhoneNumber') here

});
}
},
);
} else {
const {recentReports, personalDetails, userToInvite} = getSearchOptions(
this.props.reports,
this.props.personalDetails,
searchValue,
);
this.setState(prevState => ({
userToInvite,
recentReports,
personalDetails,
headerMessage: prevState.recentReports.length + prevState.personalDetails.length
!== 0 ? this.props.translate('messages.noEmailOrPhone') : '',
}));
}
}

render() {
const sections = this.getSections();

return (
<ScreenWrapper>
<HeaderWithCloseButton
Expand All @@ -151,23 +213,25 @@ class SearchPage extends Component {
value={this.state.searchValue}
onSelectRow={this.selectReport}
onChangeText={(searchValue = '') => {
const {
recentReports,
personalDetails,
userToInvite,
} = getSearchOptions(
this.props.reports,
this.props.personalDetails,
searchValue,
);
this.setState({
searchValue,
userToInvite,
recentReports,
personalDetails,
});
this.setState({searchValue});

// Clears the header message on clearing the input
if (!searchValue) {
this.validateInput.cancel();
setTimeout(() => {
this.setState({
headerMessage: '',
userToInvite: null,
recentReports: this.preserveRecentReports,
});
}, 0);
} else {
this.validateInput(searchValue);
}
}}
headerMessage={headerMessage}
headerMessage={
this.state.headerMessage
}
hideSectionHeaders
hideAdditionalOptionStates
showTitleTooltip
Expand Down Expand Up @@ -195,5 +259,8 @@ export default compose(
session: {
key: ONYXKEYS.SESSION,
},
countryCode: {
key: ONYXKEYS.COUNTRY_CODE,
},
}),
)(SearchPage);
5 changes: 5 additions & 0 deletions src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ const styles = {
fontSize: variables.fontSizeSmall,
},

textMicroGTA: {
fontSize: variables.fontSizeSmall,
fontFamily: fontFamily.GTA,
},

Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete if you're not using anymore

textMicroBold: {
color: themeColors.text,
fontWeight: fontWeightBold,
Expand Down
Loading