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

Use shared code for emoji typeahead #4636

Closed
gnprice opened this issue Apr 10, 2021 · 2 comments · Fixed by #5359
Closed

Use shared code for emoji typeahead #4636

gnprice opened this issue Apr 10, 2021 · 2 comments · Fixed by #5359
Assignees
Labels
a-compose/send Compose box, autocomplete, camera/upload, outbox, sending a-emoji @zulip/shared Good spots to share code with the webapp

Comments

@gnprice
Copy link
Member

gnprice commented Apr 10, 2021

We'd like to switch to sharing the webapp's code for the emoji typeahead. This will let us fix various aspects of that UI all at once, like #3833 -- and fundamentally it will be easier for the Zulip project to maintain one high-quality implementation of choosing what emoji you might mean as you're typing a message, rather than try to maintain two.

The work on the webapp side has already been done (with zulip/zulip#13739 and some followups shortly after) to free that code of webapp-specific dependencies and to move it into our shared package, in the file typeahead.js there. So what's needed now is the work on the mobile side to start using that.

We've also previously taken a different piece of webapp code (the typing-status implementation) all the way to using it in the mobile app, so we know we have all the infrastructure for this set up and working end to end. We've only done it that once, though, so there may still be rough edges or gaps in our docs.

Here's the next steps, as described at zulip/zulip#13728:

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

  • [done!] Move the relevant code into static/shared/. […]
  • 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.

So we need to bump the version of @zulip/shared that we use, and adapt our code to use the shared functions typeahead.get_emoji_matcher and typeahead.sort_emojis in place of our own implementations of the same functionality.

In the process of doing that, we may see refactors we want to make to the shared code and perhaps the webapp code that uses it, and if so we can do those along the way.

For how to go about doing this:

  • See our docs on the @zulip/shared workflow:
    • the README of the @zulip/shared package's tree, which discusses how it's deployed;
    • docs/howto/yarn-link.md, which describes a key piece of dev workflow for developing changes in @zulip/shared together with the mobile app.
  • See the four PRs (two zulip-mobile, two zulip/zulip) I linked in the quote above for an example (but note they include some work we only had to do once, and some work on the zulip/zulip side that corresponds to what's already been done now for emoji typeahead.)
    • One thing we'll definitely want to do in zulip/zulip is to add types for the interface of this shared code; see zulip/zulip@b70b7df for the previous example of that.
@gnprice gnprice added a-compose/send Compose box, autocomplete, camera/upload, outbox, sending P1 high-priority a-emoji @zulip/shared Good spots to share code with the webapp labels Apr 10, 2021
@WesleyAC
Copy link
Contributor

Removing P1 from this, as we discussed several months ago (I now have experience working in @zulip/shared, which was the reason this was P1)

@gnprice
Copy link
Member Author

gnprice commented Apr 19, 2022

This was partly done in #5326 -- in particular the sorting step is now from shared code. Still to do is the filtering step.

gnprice added a commit to gnprice/zulip that referenced this issue Apr 21, 2022
For example, if a user's name is "Simon Peyton Jones", we'll already
match that name on the queries "Pey" or "Peyton", as well as on
"Simon P".  We should do so on "Peyton J" or "Peyton Jones", too.

Similarly, if the user is looking for an emoji of a face in the moon
and they start by typing ":moon", we'll show them both 🌝 "moon face"
and 🌚 "new moon face", along with some other moon-related results.
If they go on to make it ":moon " or ":moon f", though -- as one very
naturally would in order to eliminate things like "waxing moon" and
"moon ceremony" -- then we mysteriously eliminate 🌚 "new moon face".
Instead, the query "moon f" should match both 🌚 and 🌝.

Found this while comparing the web/shared implementation with the
mobile implementation of emoji search.  The new behavior here
reflects what we already do for emoji search in mobile, both in the
compose box's typeahead and in the add-a-reaction screen.  The
existing behavior here seems pretty annoying, so fixing it will be
part of switching on mobile to the shared code (zulip/zulip-mobile#4636)
without regressing the user experience.
gnprice added a commit to gnprice/zulip that referenced this issue Apr 21, 2022
For example, if a user's name is "Simon Peyton Jones", we'll already
match that name on the queries "Pey" or "Peyton", as well as on
"Simon P".  We should do so on "Peyton J" or "Peyton Jones", too.

Similarly, if the user is looking for an emoji of a face in the moon
and they start by typing ":moon", we'll show them both 🌝 "moon face"
and 🌚 "new moon face", along with some other moon-related results.
If they go on to make it ":moon " or ":moon f", though -- as one very
naturally would in order to eliminate things like "waxing moon" and
"moon ceremony" -- then we mysteriously eliminate 🌚 "new moon face".
Instead, the query "moon f" should match both 🌚 and 🌝.

Found this while comparing the web/shared implementation with the
mobile implementation of emoji search.  The new behavior here
reflects what we already do for emoji search in mobile, both in the
compose box's typeahead and in the add-a-reaction screen.  The
existing behavior here seems pretty annoying, so fixing it will be
part of switching on mobile to the shared code (zulip/zulip-mobile#4636)
without regressing the user experience.

The current behavior was introduced, more or less, in 245d65e; then
revised in 5edbcb8 to make the logic more clear, and a fix made in
542f476, all 2018.  The PR thread was zulip#8286, following issue zulip#8279.
The old behavior before those changes was pure substring matching,
plus a trailing space was ignored (which is the part the issue was
about.)  None of the discussion touches on this question; as far as I
can tell, the fact that "Peyton J" doesn't match "Simon Peyton Jones",
nor "moon " match "new moon face", was entirely an unintentional
side effect of those changes.
timabbott pushed a commit to zulip/zulip that referenced this issue Apr 22, 2022
For example, if a user's name is "Simon Peyton Jones", we'll already
match that name on the queries "Pey" or "Peyton", as well as on
"Simon P".  We should do so on "Peyton J" or "Peyton Jones", too.

Similarly, if the user is looking for an emoji of a face in the moon
and they start by typing ":moon", we'll show them both 🌝 "moon face"
and 🌚 "new moon face", along with some other moon-related results.
If they go on to make it ":moon " or ":moon f", though -- as one very
naturally would in order to eliminate things like "waxing moon" and
"moon ceremony" -- then we mysteriously eliminate 🌚 "new moon face".
Instead, the query "moon f" should match both 🌚 and 🌝.

Found this while comparing the web/shared implementation with the
mobile implementation of emoji search.  The new behavior here
reflects what we already do for emoji search in mobile, both in the
compose box's typeahead and in the add-a-reaction screen.  The
existing behavior here seems pretty annoying, so fixing it will be
part of switching on mobile to the shared code (zulip/zulip-mobile#4636)
without regressing the user experience.

The current behavior was introduced, more or less, in 245d65e; then
revised in 5edbcb8 to make the logic more clear, and a fix made in
542f476, all 2018.  The PR thread was #8286, following issue #8279.
The old behavior before those changes was pure substring matching,
plus a trailing space was ignored (which is the part the issue was
about.)  None of the discussion touches on this question; as far as I
can tell, the fact that "Peyton J" doesn't match "Simon Peyton Jones",
nor "moon " match "new moon face", was entirely an unintentional
side effect of those changes.
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Apr 25, 2022
This applies both to the typeahead in compose after you type a `:`,
and to the emoji search screen when you go to add a reaction to a
message.

The notable behavior change this gets us is nice behavior on spaces:
in your query you can now separate words with spaces, rather than
having to type underscores.  (It still works the same if you do type
an underscore.)

Fixes: zulip#4636
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-compose/send Compose box, autocomplete, camera/upload, outbox, sending a-emoji @zulip/shared Good spots to share code with the webapp
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants