App icon indicating copy to clipboard operation
App copied to clipboard

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

Open lakchote opened this issue 1 year ago • 37 comments

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 OwnerCurrent Issue Owner: @

lakchote avatar Oct 16 '24 19:10 lakchote

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

289Adam289 avatar Oct 18 '24 08:10 289Adam289

Not overdue, on hold for https://github.com/Expensify/react-native-live-markdown/pull/439.

ikevin127 avatar Oct 21 '24 18:10 ikevin127

Still on hold for https://github.com/Expensify/react-native-live-markdown/pull/439.

ikevin127 avatar Oct 24 '24 02:10 ikevin127

Still on hold for https://github.com/Expensify/react-native-live-markdown/pull/439.

ikevin127 avatar Oct 28 '24 21:10 ikevin127

Still on hold for https://github.com/Expensify/react-native-live-markdown/pull/439.

ikevin127 avatar Nov 01 '24 00:11 ikevin127

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

Same.

lakchote avatar Nov 04 '24 11:11 lakchote

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 https://github.com/Expensify/react-native-live-markdown/pull/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: https://github.com/Expensify/App/issues/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

Kicu avatar Nov 05 '24 13:11 Kicu

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

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

blazejkustra avatar Nov 05 '24 13:11 blazejkustra

Nice! Thanks for the detailed updated @Kicu! Looking forward to the PR and testing!

luacmartins avatar Nov 05 '24 19:11 luacmartins

Not overdue, progress still happening!

lakchote avatar Nov 11 '24 10:11 lakchote

Not overdue, progress still happening!

Same

lakchote avatar Nov 13 '24 10:11 lakchote

Quick update because this is taking some time. I tested the liveMarkdown PR with worklets and it worked great on - I was able to actually use live markdown and pass it down any JS code as a parser 👍

Unfortunately this breaks on mobile, and @tomekzaw needs to spent a bit more fixing bugs. I will post another update next week, meanwhile we need to bump some other libraries to be ready for worklets - which other people are doing.

Kicu avatar Nov 14 '24 14:11 Kicu

I appreciate the update @Kicu!

luacmartins avatar Nov 14 '24 17:11 luacmartins

Unfortunately this breaks on mobile, and @tomekzaw needs to spent a bit more fixing bugs.

This bug should be fixed with https://github.com/software-mansion/react-native-reanimated/pull/6706, we'll test it out in E/App early next week.

tomekzaw avatar Nov 15 '24 16:11 tomekzaw

@lakchote, @luacmartins, @ikevin127 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Nov 18 '24 09:11 melvin-bot[bot]

Not overdue, needs retest once PR mentioned in https://github.com/Expensify/App/issues/50949#issuecomment-2479338167 is merged.

ikevin127 avatar Nov 18 '24 10:11 ikevin127

PR got merged, retest can be done.

lakchote avatar Nov 21 '24 13:11 lakchote

Did we bump the version in App already?

luacmartins avatar Nov 21 '24 17:11 luacmartins

Not yet, PR is still in progress. We'll update with more info tomorrow 👍

blazejkustra avatar Nov 21 '24 19:11 blazejkustra

Update: We need to wait for Reanimated 3.16.3, which includes a minor fix for shared value mocks that's necessary to proceed (tests are currently failing). It should be ready early next week, but if not, we can overwrite the mock ourselves = meaning it won't hold this task anyway.

blazejkustra avatar Nov 22 '24 14:11 blazejkustra

Meanwhile a few days ago an important PR for live-markdown was merged, that moved it from yarn to npm: https://github.com/Expensify/react-native-live-markdown/pull/539

There is a lot of things happening in live-markdown now, but the worklets PR is very close to done and we will be pushing it this week.

Kicu avatar Nov 25 '24 08:11 Kicu

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

melvin-bot[bot] avatar Nov 25 '24 09:11 melvin-bot[bot]

Updates are just above.

lakchote avatar Nov 25 '24 09:11 lakchote

Update: Reanimated bump is ready for review 🚀

blazejkustra avatar Nov 26 '24 10:11 blazejkustra

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] avatar Nov 28 '24 14:11 melvin-bot[bot]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.67-9 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

  • https://github.com/Expensify/App/pull/52960

If no regressions arise, payment will be issued on 2024-12-05. :confetti_ball:

For reference, here are some details about the assignees on this issue:

  • @ikevin127 requires payment (Needs manual offer from BZ)

melvin-bot[bot] avatar Nov 28 '24 14:11 melvin-bot[bot]

@luacmartins Mind assigning BZ here for payment ? I'd appreciate it.

ikevin127 avatar Dec 04 '24 20:12 ikevin127

Job added to Upwork: https://www.upwork.com/jobs/~021864412596340849116

melvin-bot[bot] avatar Dec 04 '24 20:12 melvin-bot[bot]

Current assignee @ikevin127 is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] avatar Dec 04 '24 20:12 melvin-bot[bot]

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] avatar Dec 04 '24 20:12 melvin-bot[bot]