Skip to content
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

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

Open
lakchote opened this issue Oct 16, 2024 · 63 comments
Open
Assignees
Labels
Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@lakchote
Copy link
Contributor

lakchote commented Oct 16, 2024

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: @
Issue OwnerCurrent Issue Owner: @289Adam289
@lakchote lakchote moved this to Release 3: Fall 2024 (Nov) in [#whatsnext] #expense Oct 16, 2024
@luacmartins luacmartins changed the title [Search] [Autocomplete] Highlight autocomplete key and selected value [Search v2.5] [Autocomplete] Highlight autocomplete key and selected value Oct 16, 2024
@luacmartins luacmartins added Daily KSv2 and removed Weekly KSv2 labels Oct 16, 2024
@289Adam289
Copy link
Contributor

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.

@luacmartins luacmartins changed the title [Search v2.5] [Autocomplete] Highlight autocomplete key and selected value [HOLD live-markdown 439][Search v2.5] [Autocomplete] Highlight autocomplete key and selected value Oct 18, 2024
@melvin-bot melvin-bot bot added the Overdue label Oct 21, 2024
@ikevin127
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 21, 2024
@ikevin127
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 24, 2024
@ikevin127
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 28, 2024
@ikevin127
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Nov 1, 2024
@lakchote
Copy link
Contributor Author

lakchote commented Nov 4, 2024

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

Same.

@Kicu
Copy link
Contributor

Kicu commented Nov 5, 2024

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
Copy link
Contributor

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

@Kicu
Copy link
Contributor

Kicu commented Dec 16, 2024

Yup one of us will start working on this task this week. More updates will come soon

@289Adam289
Copy link
Contributor

Hi, I am Adam from SWM and I would like to work on this issue.

@luacmartins
Copy link
Contributor

Nice, really excited for this one. Thanks for working on it @289Adam289!

Copy link

melvin-bot bot commented Dec 20, 2024

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

@289Adam289
Copy link
Contributor

Update:

I've been working on this issue and I'd like to share where we're at. Although is it WIP I’ve made significant progress. Below are some issues encountered and the advances made:

  1. Worklets Compatibility: The parsers generated by Peggy use syntax incompatible with worklets (function.prototype), which complicates workletization. Thus, I wrote a script that modifies the autocomplete.js file.

  2. Handling Spaces in LiveMarkdownTextInput: On the web, LiveMarkdownTextInput is implemented using a "contenteditable" div. LiveMarkdown simulates single-line input by using different css props e.g.white-space: nowrap which collapses excess spaces and replaces them with &nbsp;. I have improved the parser to work with both types of spaces and changed the autocomplete values to include a trailing non-break space, which is not removed by white-space: nowrap.

  3. Selection Logic in LiveMarkdown: Currently, LiveMarkdown on the web lacks the functionality to adjust the selection dynamically as input length changes. I have implemented this feature, which was necessary only for the web version as the native implementation utilizes different native components.

  4. Styling: I am currently focusing on improving the styling for web implementation. Markdown formatting is slightly moved in relation to the text, which is probably LM bug. @Kicu has already discussed it with LiveMarkdown team.

Next Steps:

  1. Improve styling on web.
  2. Implement different markdown formatting depending on autocomplete key.

Discussion Point:

It might be a good time to talk about how we format markdown for different types of keys. Right now, everything recognized by the autocomplete parsers gets the same blue highlight like "user-mention." Should we keep it this way, or try something different for various keys?
Looking forward to your thoughts and any further directions on how to proceed.
cc @luacmartins for thoughts

Videos to show how things look right now:

Screen.Recording.2024-12-20.at.13.17.04.mov
Screen.Recording.2024-12-20.at.13.16.17.mov
Screen.Recording.2024-12-20.at.15.40.35.mov

@melvin-bot melvin-bot bot removed the Overdue label Dec 20, 2024
@garrettmknight garrettmknight moved this from Hold for Payment to Bugs and Follow Up Issues in [#whatsnext] #expense Dec 23, 2024
Copy link

melvin-bot bot commented Dec 24, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Dec 24, 2024
Copy link

melvin-bot bot commented Dec 26, 2024

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

@Kicu
Copy link
Contributor

Kicu commented Dec 30, 2024

we'll be back Jan 7th, and work on this will continue

Copy link

melvin-bot bot commented Dec 30, 2024

@Kicu, @sonialiap, @lakchote, @luacmartins, @ikevin127, @289Adam289 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

@luacmartins
Copy link
Contributor

@289Adam289 thanks for the update!

It might be a good time to talk about how we format markdown for different types of keys. Right now, everything recognized by the autocomplete parsers gets the same blue highlight like "user-mention." Should we keep it this way, or try something different for various keys?
Looking forward to your thoughts and any further directions on how to proceed.
cc @luacmartins for thoughts

The doc doesn't mention any specific styles, but the mocks seem to follow the mentions styles, i.e. everything has a blue highlight, except your own mention, which is green. So I think we can keep the same here and have everything blue, except for your own email/display name which should be green.

Copy link

melvin-bot bot commented Jan 1, 2025

@Kicu, @sonialiap, @lakchote, @luacmartins, @ikevin127, @289Adam289 10 days overdue. I'm getting more depressed than Marvin.

@luacmartins
Copy link
Contributor

@289Adam289 will resume work once he's back from ooo (maybe on Jan 7th?)

Copy link

melvin-bot bot commented Jan 3, 2025

@Kicu, @sonialiap, @lakchote, @luacmartins, @ikevin127, @289Adam289 12 days overdue now... This issue's end is nigh!

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jan 6, 2025
Copy link

melvin-bot bot commented Jan 6, 2025

This issue has not been updated in over 14 days. @Kicu, @sonialiap, @lakchote, @luacmartins, @ikevin127, @289Adam289 eroding to Weekly issue.

@melvin-bot melvin-bot bot removed the Overdue label Jan 6, 2025
@luacmartins luacmartins added Daily KSv2 and removed Weekly KSv2 labels Jan 6, 2025
@luacmartins
Copy link
Contributor

This is still a daily. SWM is coming back tomorrow and will resume work here.

@289Adam289
Copy link
Contributor

Update:

Right now iOS and Android work well. Color of a highlight changes to green for current user mention. The only thing left are some cursor position and background alignment issues on web that are being worked on by @BartoszGrajdek in Expensify/react-native-live-markdown#594.
I think we should wait for that LM pr. What do you think @luacmartins?

Current state:
Web:

Screen.Recording.2025-01-08.at.12.42.10.mp4

Android:

Screen.Recording.2025-01-08.at.12.39.39.mp4

iOS:

Screen.Recording.2025-01-08.at.12.32.46.mp4

@luacmartins
Copy link
Contributor

Yea, I agree that we should wait for that PR to fix the alignment. Additionally, gonna tag @Expensify/design for more input on the highlight styles.

@dannymcclain
Copy link
Contributor

I think the highlights are looking pretty good but it would be ideal if we could match them exactly to the styles we use for mention highlights (mainly thinking the border radius and subtle spacing).

Also this might be off topic so feel free to ignore if it is, but will the highlight always happen right away? I think I would expect it to only highlight if we found a legitimate match for the query—but I could be wrong about that. Will let the other designers gut check me there.

@dubielzyk-expensify
Copy link
Contributor

Agree with both of Danny's comments. I also kinda expect it to only show highlight once it's matched, but I don't think it's suuuper critical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
Status: Bugs and Follow Up Issues
Development

No branches or pull requests