Highlight autocomplete value#54403
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
src/libs/SearchAutocompleteUtils.ts
Outdated
| return newQuery; | ||
| } | ||
|
|
||
| function workletizedParser(input: string) { |
There was a problem hiding this comment.
Just noticed that this name is rather weird sounding, so I hope we can change it.
|
This is shaping up really nicely! Great work so far! |
There was a problem hiding this comment.
Went over the PR for the final time and it looks great 👍
I just have 2 last thoughts to improve this:
- my comment about duplication in SearchRouterInput
- actually I think this is a great time to rename this component, since we are doing some refactoring on it anyways. It's used both on Search results page and in the router, I think we could name it something much more generic. Perhaps simply
SearchInputorSearchAutocompleteInputis a better name?
I want other devs to associate this component with searching in general, not specifically with the router
|
Nice! Thanks for addressing my comments. I agree that we can leave the border radius out for now so things are consistent across platforms. Additionally, a follow up to handle both syntax key styles and the exact query match highlights sounds good! |
|
I'll review again shortly! |
luacmartins
left a comment
There was a problem hiding this comment.
Nice! Code LGTM and tests well! Thanks for the hard work!
|
@ikevin127 all yours |
|
🎉 LGTM, good work on this one! Thanks for promptly addressing all issues 👍 @289Adam289 Can you please resolve the conflicts ? Then I'll do a final check before 🟢 Approving. |
|
Everything should work now. cc @luacmartins @ikevin127 |
|
@289Adam289 we have conflicts again |
ikevin127
left a comment
There was a problem hiding this comment.
♻️ Reviewed the latest updates and it LGTM 🚀
Given that the conflicts can be cleanly merged, I approved.
@luacmartins feel free to merge this as soon as the conflicts are resolved, but keep an eye on this merge freeze Slack announcement from Rory before merging.
ℹ️ I noticed this thing, which is probably not a blocker but wanted to mention it:
-
now both primary and secondary account emails (contact methods) are highlighted with green when inputted which is great, but when pressing Enter to actually perform search based in the input values, only the primary email will transform into
{displayName}, while the secondary email remains as the email address.See video
WEB.mov
e4b4a9d to
601aeb9
Compare
|
Failing checks are for hybrid with message: |
|
@luacmartins looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@luacmartins QA still ongoing on this one or how come it wasn't deployed yet ? 🤔 |
|
The last checklist was created on Jan 31st, before this PR was merged. We're waiting for the next deploy for it to hit staging. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.94-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.0.94-25 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.0.94-25 🚀
|
Explanation of Change
This PR implements Search syntax highlighting using latest version of
react-native-live-markdownlibrary.Autocomplete parser is currently parametrized with current user and workletized to be passed as
parserprop toRNLMTextInputcomponent.Fixed Issues
$#50949
PROPOSAL:
Tests
from:user@user.com)Offline tests
QA Steps
from:user@user.com)PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Android: Native
Screen.Recording.2025-01-08.at.12.39.39.mp4
Android: mWeb Chrome
Screen.Recording.2025-01-28.at.15.09.25.mp4
iOS: Native
Screen.Recording.2025-01-08.at.12.32.46.mp4
iOS: mWeb Safari
Screen.Recording.2025-01-28.at.15.07.13.mp4
MacOS: Chrome / Safari
Screen.Recording.2025-01-28.at.15.10.55.mp4
MacOS: Desktop