-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Updated country and state searches to fix extra spaces + support searching with/without non alphabet chars in country name #24140
Updated country and state searches to fix extra spaces + support searching with/without non alphabet chars in country name #24140
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
@eVoloshchak Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
return []; | ||
} | ||
|
||
return _.filter(data, (country) => country.text.toLowerCase().includes(searchValue.toLowerCase())); | ||
return _.filter(data, (country) => country.searchValue.includes(searchValueWithOnlyAlphabets) || country.value.toLowerCase().includes(searchValueWithOnlyAlphabets)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From here
I think we could also add the ability to search for a country by entering a country code, it's reasonable for users to expect this to work in a country picker
Searching for both country name and code now
@@ -38,12 +38,12 @@ const defaultProps = { | |||
}; | |||
|
|||
function filterOptions(searchValue, data) { | |||
const trimmedSearchValue = searchValue.trim(); | |||
if (trimmedSearchValue.length === 0) { | |||
const searchValueWithOnlyAlphabets = searchValue.toLowerCase().replaceAll(' ', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The USA States only had spaces in names and no other special char (in my tests), so only replaced empty spaces. Plz LMK if there is anything missing here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use the same logic here too, in essence, it should work even if there are states with special characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, will update that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, now both country/state search work the same (we can probably make a utility out of it)
return []; | ||
} | ||
|
||
return _.filter(data, (country) => country.text.toLowerCase().includes(searchValue.toLowerCase())); | ||
return _.filter(data, (countryState) => countryState.searchValue.includes(searchValueWithOnlyAlphabets) || countryState.value.toLowerCase().includes(searchValueWithOnlyAlphabets)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Searching for both state name and code, same reason as this
Gentle bump for review @eVoloshchak |
Bump for review @eVoloshchak |
Gentle bump for review @eVoloshchak |
Bump for review @eVoloshchak |
Reviewing today |
src/CONST.js
Outdated
@@ -1165,6 +1165,7 @@ const CONST = { | |||
CARD_EXPIRATION_DATE: /^(0[1-9]|1[0-2])([^0-9])?([0-9]{4}|([0-9]{2}))$/, | |||
PAYPAL_ME_USERNAME: /^[a-zA-Z0-9]{1,20}$/, | |||
ROOM_NAME: /^#[a-z0-9à-ÿ-]{1,80}$/, | |||
COUNTRY_NAME_WITH_ONLY_ALPHABETS: /[-'().&\s]/g, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this the other way around and exclude all non-alphabetic characters. We have to also remember there's a Spanish locale
COUNTRY_NAME_WITH_ONLY_ALPHABETS: /[-'().&\s]/g, | |
NON_ALPHABETIC_AND_NON_LATIN_CHARS: /[^a-zA-ZÀ-ÿ]/g, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, Implemented
return []; | ||
} | ||
|
||
return _.filter(data, (country) => country.text.toLowerCase().includes(searchValue.toLowerCase())); | ||
return _.filter(data, (country) => country.searchValue.includes(searchValueWithOnlyAlphabets) || country.value.toLowerCase().includes(searchValueWithOnlyAlphabets)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be reduced to
return _.filter(data, (country) => country.searchValue.includes(searchValueWithOnlyAlphabets) || country.value.toLowerCase().includes(searchValueWithOnlyAlphabets)); | |
return _.filter(data, (country) => `${country.value}${country.searchValue}`.toLowerCase().includes(trimmedSearchValue)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can sort it after filtering
_.sortBy(filteredData, (country) => country.value.toLowerCase() === searchValueWithOnlyAlphabets ? -1 : 1);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -52,6 +52,7 @@ function CountrySelectorModal({currentCountry, isVisible, onClose, onCountrySele | |||
keyForList: countryISO, | |||
text: countryName, | |||
isSelected: currentCountry === countryISO, | |||
searchValue: countryName.toLowerCase().replaceAll(CONST.REGEX.COUNTRY_NAME_WITH_ONLY_ALPHABETS, ''), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, nevermind, this is necessary
Removing it would break cases with countries that have special characters in their name
(but toLowerCase()
isn't needed anymore)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I added it here because if we store it in the state we won't have to recalculate it. But I will remove it if you think it's not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it here because if we store it in the state we won't have to recalculate it.
Fair enough, let's leave it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -38,12 +38,12 @@ const defaultProps = { | |||
}; | |||
|
|||
function filterOptions(searchValue, data) { | |||
const trimmedSearchValue = searchValue.trim(); | |||
if (trimmedSearchValue.length === 0) { | |||
const searchValueWithOnlyAlphabets = searchValue.toLowerCase().replaceAll(' ', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use the same logic here too, in essence, it should work even if there are states with special characters
@@ -34,12 +34,12 @@ const defaultProps = { | |||
}; | |||
|
|||
function filterOptions(searchValue, data) { | |||
const trimmedSearchValue = searchValue.trim(); | |||
if (trimmedSearchValue.length === 0) { | |||
const searchValueWithOnlyAlphabets = searchValue.toLowerCase().replaceAll(CONST.REGEX.COUNTRY_NAME_WITH_ONLY_ALPHABETS, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think trimmedSearchValue
is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I think we have to keep it since most searches won't work if we remove ex: U.S Virgin Islands -> no results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean just the name, so it looks like this
const trimmedSearchValue = searchValue.toLowerCase().replaceAll(CONST.REGEX.NON_ALPHABETIC_AND_NON_LATIN_CHARS, '');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, sry I misunderstood.
I updated test videos to include the new |
function filterOptions(searchValue, data) { | ||
const trimmedSearchValue = searchValue.trim(); | ||
if (trimmedSearchValue.length === 0) { | ||
function searchOptions(searchValue, data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are searchOptions
identical for CountrySelectorModal and StateSelectorModal? If so, we could move this function to libs/CountrySelectorUtils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, searchOptions
logic is the same for both CountrySelectorModal and StateSelectorModal (other than some variable name and comments), I will move it to libs/CountrySelectorUtils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this exports a single item, let's just call it searchCountryOptions.js
(a single file, no need for a folder)
import CONST from '../../CONST'; | ||
|
||
/** | ||
* Searches the countriesData and returns sorted results based on country code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Searches the countriesData and returns sorted results based on country code | |
* Searches the countries or states data and returns sorted results based on the search query |
* Searches the countriesData and returns sorted results based on country code | ||
* @param {String} searchValue | ||
* @param {Object[]} countriesData - An array of country data objects | ||
* @returns {Object[]} An array of sorted country data based on country code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @returns {Object[]} An array of sorted country data based on country code | |
* @returns {Object[]} An array of countries/states sorted based on the search query |
* @param {Object[]} countriesData - An array of country data objects | ||
* @returns {Object[]} An array of sorted country data based on country code | ||
*/ | ||
function searchOptions(searchValue, countriesData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function searchOptions(searchValue, countriesData) { | |
function searchCountryOptions(searchValue, countriesData) { |
Applied the change @eVoloshchak |
@@ -6,6 +6,7 @@ import useLocalize from '../../hooks/useLocalize'; | |||
import HeaderWithBackButton from '../HeaderWithBackButton'; | |||
import SelectionListRadio from '../SelectionListRadio'; | |||
import Modal from '../Modal'; | |||
import searchOptions from '../../libs/searchCountryOptions'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import searchOptions from '../../libs/searchCountryOptions'; | |
import searchCountryOptions from '../../libs/searchCountryOptions'; |
@@ -6,6 +6,7 @@ import Modal from '../Modal'; | |||
import HeaderWithBackButton from '../HeaderWithBackButton'; | |||
import SelectionListRadio from '../SelectionListRadio'; | |||
import useLocalize from '../../hooks/useLocalize'; | |||
import searchOptions from '../../libs/searchCountryOptions'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import searchOptions from '../../libs/searchCountryOptions'; | |
import searchCountryOptions from '../../libs/searchCountryOptions'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a thought about this too but searchCountryOptions
in StateSelectorModal.js
sounded a little misleading 😅. Anyways updating it as well.
Updated @eVoloshchak |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-08-08.at.00.55.15.movMobile Web - Chromescreen-20230808-010553.mp4Mobile Web - SafariScreen.Recording.2023-08-08.at.00.59.25.movDesktopScreen.Recording.2023-08-08.at.00.57.10.moviOSThere is an issue preventing me from logging in on iOS (endless spinner after entering the magic code). There is no platform-specific code, should work the same on iOS Androidscreen-20230808-010359.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@huzaifa-99, thanks for the patience and quick turnaround time!
Always welcomed @eVoloshchak. And thank you for the nice suggestions and quick feedback! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.52-0 🚀
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.52-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.52-5 🚀
|
Details
Fixed Issues
$ #23727
PROPOSAL: #23727 (comment)
Tests
Myanmar (Burma), Cocos (Keeling) Islands
-- Verify that it worksSt. Lucia, U.S Virgin Islands
-- Verify that it worksCongo - Kinshasa, Congo - Brazzaville, Timor-Leste
-- Verify that it worksSt. Kitts & Nevis, St. Vincent & Grenadines
-- Verify that it worksCôte d'Ivoire
-- Verify that it worksAF -> Afganistan, US -> United States, DZ -> Argeliz
-- Verify that it worksNY -> New York, DC -> District Of Columbia
-- Verify that it worksOffline tests
Same as "Tests" section above
QA Steps
Same as "Tests" section above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Chrome:
Web.Chrome.mp4
Web.Chrome.mp4
Safari:
Web.Safari.mp4
Web.Safari.mp4
Mobile Web - Chrome
mWeb.Chrome.mp4
mWeb.Chrome.mp4
Mobile Web - Safari
mWeb.Safari.mp4
mWeb.Safari.mp4
Desktop
Desktop.Native.mp4
Desktop.Native.mp4
iOS
IOS.Native.mp4
IOS.Native.mp4
Android
Android.Native.mp4
Android.Native.mp4