[23543] Multi words zones now work while searching for timezones.#25569
Conversation
|
I need to add Screenshots/Videos, but I firstly wanted to agree on the desired functionality. This version works as follows:
The only downsides for now are that " @fedirjh Would you say that it's ok as is, or would you want me to make some changes? If so, please tell me what changes. I'll come back with a potential fix & a list of any new downside. Thank you! |
fedirjh
left a comment
There was a problem hiding this comment.
Left a few comments about the code.
The only downsides for now are that "IsleOfMan" dows not work, and that "East I" shows "US/East-Indiana" at the end of the list. I've tried some changes, but I seem to lose some other functionality by fixing these.
@DanutGavrus Let's use the same regex in the linked proposal, I left a suggestion about normalizing the searchText and I think that will resolve those issues, just let's make sure to use the right regex .replace(/[^a-z0-9\+\-\/]/g, '') for the timezone names.
| setTimezoneOptions(_.filter(allTimezones.current, (tz) => tz.text.toLowerCase().includes(searchText.trim().toLowerCase()))); | ||
| setTimezoneOptions(_.filter(allTimezones.current, (tz) => { | ||
| let shouldShow = true; | ||
| searchText.toLowerCase().replace(/[^a-z0-9]/g, ' ').split(' ').forEach((word) => { |
There was a problem hiding this comment.
I think we can sanitize the searchText before checking :
const searchWords = searchText.toLowerCase().replace(/[^a-z0-9\+\-\s+/]/g, '').split(/\s+/);There was a problem hiding this comment.
If we keep the current regex, this won't be needed, otherwise, I'll update as suggested.
| if (tz.text.toLowerCase().replace(/[^a-z0-9]/g, ' ').indexOf(word) < 0) shouldShow = false; | ||
| }) | ||
| return shouldShow; |
There was a problem hiding this comment.
We have a lint here.
Can't we use Array.prototype.every() instead of .forEach, I think this will simplify the code and make it more readable.
There was a problem hiding this comment.
Run npm run prettier, fixed several lint issues & used Array.prototype.every() instead of .forEach.
|
@fedirjh Based on the review comments, I think this will be the new regex(not the final code, just quickly testing the regex): const searchWords = searchText.toLowerCase().replace(/[^a-z0-9\+\-\s+/]/g, '').split(/\s+/);
searchWords.forEach((word) => {
if (tz.text.toLowerCase().replace(/[^a-z0-9\+\-\/]/g, '').indexOf(word) < 0) shouldShow = false;
})
If my comparison is right, we gain "IsleOfMan" for "Eur/Isl", and "the result is shown at the end of the list for some searches issue" persists. I think the last issue is not very big, inserting 1 more letter eventually shows the result at the top. What would you say? |
|
@fedirjh Isn't this regex: const searchWords = searchText.toLowerCase().replace(/[^a-z0-9]/g, ' ').split(' ');
searchWords.forEach((word) => {
if (tz.text.toLowerCase().replace(/[^a-z0-9]/g, ' ').indexOf(word) < 0) shouldShow = false;
})Simpler, more future proof, and with almost the same functionality? We don't exactly specify the special characters(+, -, /) here, maybe some new ones will appear in the future. Only diff in functionality is that "IsleOfMan" does not work and "Eur/Isl" does. Personally, I think I'd prefer this, but let me know which one you want to keep & I'll make a new commit. Thanks! |
|
@fedirjh Added Web recording for easier understanding of the current changes. |
|
Hey @DanutGavrus, it appears that we've made updates to the Timezone list in this PR #24446. Some timezones, such as
I don't strongly agree with that. If we were to update the timezone list in the future to include timezones like |
|
@fedirjh Yes, that PR kind of simplified the timezones list.
It will still show the GMT +1
GMT -1
GMT 1If the priority issue is to be resolved, I do agree with you, it's not future proof enough. It will be capable to show the results, but will have small problems in prioritizing them, especially for those with special characters. Same is happening in the present, but it's simply resolved by entering one more letter, as seen now for |
fedirjh
left a comment
There was a problem hiding this comment.
Looks good, left small improvement
| searchText | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9]/g, ' ') | ||
| .split(' '), |
There was a problem hiding this comment.
| searchText | |
| .toLowerCase() | |
| .replace(/[^a-z0-9]/g, ' ') | |
| .split(' '), | |
| searchText.toLowerCase().match(/[a-z0-9]+/g) || [] |
This can be simplified, Can we move to a local const to avoid repeating the same normalization process multiple times within the filter loop?
There was a problem hiding this comment.
Yees, of course! Hope I got it right.
There was a problem hiding this comment.
Hmm, It is still included in the filter loop, and should be moved outside the loop.
There was a problem hiding this comment.
🤦 Sorry about that... Should be ok now
Reviewer Checklist
Screenshots/VideosWeb
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android
|
fedirjh
left a comment
There was a problem hiding this comment.
Looks good to me and tests well.
🎀 👀 🎀 C+ reviewed
|
✋ 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/bondydaa in version: 1.3.59-0 🚀
|
|
🚀 Deployed to staging by https://github.com/bondydaa in version: 1.3.60-0 🚀
|
|
🚀 Deployed to staging by https://github.com/bondydaa in version: 1.3.60-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.59-5 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.60-3 🚀
|
Details
Improved search functionality on the select timezone page.
Fixed Issues
$ #23543
PROPOSAL: #23543 (comment)
Tests
Same as QA Steps.
Offline tests
Same as QA Steps.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
timezone_search.mp4
Mobile Web - Chrome
android_chrome.mp4
Mobile Web - Safari
Desktop
iOS
Android
android_native.mp4