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

Make typeahead code sharable with mobile app #13728

Closed
gnprice opened this issue Jan 23, 2020 · 9 comments
Closed

Make typeahead code sharable with mobile app #13728

gnprice opened this issue Jan 23, 2020 · 9 comments
Labels
area: message-editing area: mobile Mobile push notifications; features motivated by mobile; issues requiring changes in mobile code. area: refactoring

Comments

@gnprice
Copy link
Member

gnprice commented Jan 23, 2020

@timabbott remarked on zulip/zulip-mobile#3833:

It's possible that we could refactor some of [the typeahead] logic to be shared code, and that might be a good idea in the long term. One reason to consider doing something like that now is that @showell just did a big bundle bunch of work on the typeahead code for that performance work, so it's very much in our mental cache. It might be too complicated, but I think it is worth mentioning that a lot of hours have gone into typeahead sorting logic in the webapp, and we have thousands of lines of tests for that logic, and this is conceptually a pure-logic type thing where some code sharing might be feasible (potentially with some refactoring to make it possible for the mobile app to stub out modules like pm_conversations that are used to prefer users one has had PMs with until it has an analog to use).

I think this would be very fruitful! As Tim says, there's a lot of pure logic in there, and it encodes a lot of nuanced product choices that would be a lot of work to reimplement one by one in the mobile app. There's also some annoyances people regularly run into on mobile which the webapp has a nice feature that solves, like zulip/zulip-mobile#2603 and zulip/zulip-mobile#3710 -- so there's immediate gain to be had in the mobile experience by sharing this code, in addition to the maintainability benefits.

The roadmap for doing this would be a lot like our initial proof-of-concept for shared code, with typing_status.js (in #13253, #13315, zulip/zulip-mobile#3638, zulip/zulip-mobile#3662):

  • Move the relevant code into static/shared/. (For typing_status.js this was the whole small file/module; here it'd probably be parts of several files.)
    • Where the code has dependencies on other parts of our code, either move those too or refactor so it takes those dependencies as parameters.
  • On the mobile side, we'll update to a new version of the @zulip/shared NPM package (which comes from static/shared/, and replace our own implementations with calls to the shared code.
  • The process can be iterative; as we migrate in the mobile app, we may spot further refactors that'll be helpful to make to the shared code.

Probably a good strategy would be to go for one kind of typeahead/autocomplete at a time. The most valuable are probably (a) people, and (b) emoji.

I just looked through the emoji typeahead in the webapp because of zulip/zulip-mobile#3833, and I think it'd be a good, fairly easy place to start. Specifically if we could share

  • get_emoji_matcher from within composebox_typeahead
  • typeahead_helper.sort_emojis
  • all their helper functions, transitively (many of which will be re-used by other kinds of typeahead!)

I think that'd be productive -- and I think all of those appear to be pure, UI-independent functions that can be shared pretty much verbatim.

After that, we can perhaps tackle something like composebox_typeahead.get_person_suggestions. That one will take some refactoring to be shared.

@gnprice gnprice added area: message-editing area: mobile Mobile push notifications; features motivated by mobile; issues requiring changes in mobile code. labels Jan 23, 2020
@zulipbot
Copy link
Member

zulipbot commented Jan 23, 2020

Hello @zulip/server-message-view, @zulip/server-refactoring members, this issue was labeled with the "area: message-editing", "area: refactoring" labels, so you may want to check it out!

@showell
Copy link
Contributor

showell commented Jan 25, 2020

One thing we'll want to move here is people.remove_diacritics. It's in people only because that's the first place we needed it, but it has nothing to do with people data specifically, and we could move it to the new shared library.

@showell showell self-assigned this Jan 26, 2020
showell pushed a commit to showell/zulip that referenced this issue Jan 26, 2020
This extracts get_emoji_matcher and all the
functions it depended on, most of which were
in composebox_typeahead.js.

We also move remove_diacritics out of the people
module.

This is the first major step for zulip#13728.
showell pushed a commit to showell/zulip that referenced this issue Jan 26, 2020
This extracts get_emoji_matcher and all the
functions it depended on, most of which were
in composebox_typeahead.js.

We also move remove_diacritics out of the people
module.

This is the first major step for zulip#13728.
showell pushed a commit to showell/zulip that referenced this issue Jan 27, 2020
This extracts get_emoji_matcher and all the
functions it depended on, most of which were
in composebox_typeahead.js.

We also move remove_diacritics out of the people
module.

This is the first major step for zulip#13728.
timabbott pushed a commit that referenced this issue Jan 28, 2020
This extracts get_emoji_matcher and all the
functions it depended on, most of which were
in composebox_typeahead.js.

We also move remove_diacritics out of the people
module.

This is the first major step for #13728.
@chrisbobbe
Copy link
Contributor

people.remove_diacritics

I haven't loaded all this into my brain yet, but just a ping to say there's a @zulip/shared-labeled issue involving diacritics: zulip/zulip-mobile#3710 🙂

@timabbott
Copy link
Member

timabbott commented Jan 30, 2020

Yeah, that's an issue we addressed in the webapp typeahead long ago, and part of a broad category of details that it'll be very nice to share the fix for. Zulip's webapp typeahead has probably had >250 commits to it.

@zulipbot
Copy link
Member

zulipbot commented Feb 9, 2020

Hello @showell, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 14 days.

You can reclaim this issue or claim any other issue by commenting @zulipbot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

@gnprice
Copy link
Member Author

gnprice commented Jul 17, 2020

Specifically if we could share …

These pieces are all done now (from #13739, I believe), so I think for emoji typeahead the next steps are on the mobile side: zulip/zulip-mobile#3833.

Probably good for us to go and do that, for more practice doing this whole process end to end and to see if there's anything to adjust in it. But otherwise the next step on the webapp side is probably:

After that, we can perhaps tackle something like composebox_typeahead.get_person_suggestions. That one will take some refactoring to be shared.

@gnprice
Copy link
Member Author

gnprice commented Jul 18, 2020

There's also a piece of people typeahead which is in the shared package and ripe for the mobile app to start using as shared: zulip/zulip-mobile#3710

@zulipbot
Copy link
Member

zulipbot commented Mar 21, 2022

Hello @Fingel, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 14 days.

You can reclaim this issue or claim any other issue by commenting @zulipbot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

@timabbott
Copy link
Member

Closing as no longer planned given the mobile apps are being rewritten in Flutter.

@timabbott timabbott closed this as not planned Won't fix, can't repro, duplicate, stale Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: message-editing area: mobile Mobile push notifications; features motivated by mobile; issues requiring changes in mobile code. area: refactoring
Projects
None yet
Development

No branches or pull requests

7 participants