[Search v2.5] [Autocomplete] Highlight autocomplete key and selected value

[lakchote]

See https://docs.google.com/document/d/1o-Hp-tK8Z_BAcE-KRiXQicPH04qNr525EIxlG8J4RxM/edit?tab=t.0#bookmark=id.hy4h27dpoz37

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021864412596340849116
• Upwork Job ID: 1864412596340849116
• Last Price Increase: 2024-12-04

Issue Owner

Current Issue Owner: @

Issue Owner

Current Issue Owner: @289Adam289

[289Adam289]

Hi! According design doc I think this issue should be on hold for Expensify/react-native-live-markdown#439 pr that will allow our parser to be passed as worklet to live markdown.

[ikevin127]

Not overdue, on hold for Expensify/react-native-live-markdown#439.

[ikevin127]

Still on hold for Expensify/react-native-live-markdown#439.

[ikevin127]

Still on hold for Expensify/react-native-live-markdown#439.

[ikevin127]

Still on hold for Expensify/react-native-live-markdown#439.

[lakchote]

Still on hold for Expensify/react-native-live-markdown#439.

Same.

[Kicu]

Issue update

Quick reminder:
we wanted to use https://github.com/Expensify/react-native-live-markdown to implement highlighting in Search autocomplete. I recently discussed the best way to implement this with @tomekzaw who is the author of the PR.
Tomek has done a great deal of work 👏 to allow for the LiveMarkdown component to accept any JS code as parser via a prop.
Now we need to actually implement this in Expensify/App, which will require updating the version of live markdown (and some other packages) and tweaking the current code for RNMarkdownTextInput component.

This should not be a very big change in the code, but as any change it might introduce some small bugs.
In the old implementation of livemarkdown, the ExpensiMark parser was bundled together with the MarkdownTextInput component and was always used.
In the new version of MarkdownTextInput, the component accepts a parser prop, which can be any JS function adhering to correct interface.

I will try to push this forward as smoothly as possible, but we will require some help with testing.

Next steps :

1. I'm testing if the new version works correctly with Expensify locally (<--- we are here)
2. We will want to create a test build, and would need your help in getting some QAs to test it and see if there are no regressions for Composer with ExpensiMark.
3. merge Support custom parsing logic (pass worklet as parser prop) react-native-live-markdown#439 and release new version
4. bump version of react-native-live-markdown in E/App; pass expensiMark as parser for Composer, pass autocompleteParser for SearchRouter
5. Profit $$$ 😉

We will also need to bump reanimated and expensify-common (minor version for both). Thankfully @blazejkustra is already looking at bumping reanimated here: #52024 so this will speed things up.

I will open a draft PR soon for tracking the progress.
Feel free to ask anything. The workletization of parser in LiveMarkdown editor is a feature that multiple people would like to see added to the library. It will give us a lot of freedom, if we want to change how our parsers behave in future.

CC @luacmartins @lakchote @JmillsExpensify @tomekzaw @289Adam289 @SzymczakJ

[blazejkustra]

We will also need to bump reanimated and expensify-common (minor version for both). Thankfully @blazejkustra is already looking at bumping reanimated here: #52024 so this will speed things up.

I created the issue earlier today, would you mind assigning me? @lakchote

[shawnborton]

Agree with both of those comments.

Also, when you are doing autocomplete on the second item, I would expect the full query to still be there. For example, this is doing two autocompletes in Slack:

But this is what I see from the PR:

[Kicu]

@shawnborton but the top option to pick from in Search is exactly the same query - just like on slack. Do you mean you would like for every single autocomplete item to contain the whole query?
Seems kinda weird to me. What if queries get super long?

[Kicu]

It gets cut away on slack desktop app - not the best UX imo

[shawnborton]

Hmm yeah that's a fair point... though we could potentially right-align those long strings.

Either way, I don't feel too strongly, let's see if others have any strong feelings otherwise we can just go with what you have planned already.

[dannymcclain]

I don't think I feel too strongly about that one 🤔 We could always go with the current plan and see how it feels first.

[dubielzyk-expensify]

Yeah, going with the current plan and adjusting feels right to me given it's hard to predict this one

[289Adam289]

In response to the first @dannymcclain comment, the styles currently used by SearchRouter are the same as those used by Composer input. Adding border radius to the background is a bigger problem that is currently worked on in Expensify.

Regarding the issue of highlighting only when there's a legitimate query match, I think it would be safest to handle that as a follow-up. This PR is already complex, and adding further complications could increase the chance of missing regressions.

[Kicu]

RE: highlighting a mention only when it actually mentions a real login.
This is not something that we are able to easily do in the app right now, and it's a bigger task that Im currently working on in the context of Composer (the main chat input). Here is the issue: #38025

We will make this work in Search input once the above task is completed. We will also make sure that the logic and look of highlights in Search and Composer is the same.

[dannymcclain]

Thanks for the explanation—I'm fine handling that stuff as a follow up since it sounds like this PR is already pretty intense.

[shawnborton]

Yup, that totally works for me too - thanks for clarifying things.

[melvin-bot]

@Kicu, @sonialiap, @lakchote, @luacmartins, @ikevin127, @289Adam289 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@Kicu, @sonialiap, @lakchote, @luacmartins, @ikevin127, @289Adam289 Eep! 4 days overdue now. Issues have feelings too...

[Kicu]

This is being actively worked on. While Adam focused on most of this task and logic of working, I need to do a bit more work related to LiveMarkdown input to fix the styling problems. We also have colleagues from live markdown helping with this.

[luacmartins]

^ what he said

[289Adam289]

All issues with LiveMarkdown's single-line on the web are resolved. react-native-live-markdown package is being updated to a new version that includes fix in #55509

[luacmartins]

Nice! Hopefully we can get that PR merged soon and continue work here.

[Kicu]

We are also waiting for this one: #55343 but I hope it can be merged soon.
Without it the text won't be aligned vertically.

[luacmartins]

#55343 is merged! Let's keep up the momentum!

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[Kicu]

I provided fix for the styling regression after my PR, so we don't have to revert anything.
Currently waiting only on LiveMarkdown bump which fixes some more styling.

We are 90% there :)

[luacmartins]

Nice! Thanks for working on a fix. Is this the live markdown bump PR we're waiting on? #55706

[luacmartins]

Oh I see, we also have this one