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.

Sign up for GitHub

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

Jump to bottom

Pill typeahead cleanup #18592

Closed
wants to merge 3 commits into from
Closed

Pill typeahead cleanup #18592

wants to merge 3 commits into from

Conversation

m-e-l-u-h-a-n
Copy link
Member

This cleans up code in pill_typeahead.js as suggested in #17994 (comment).

Testing plan: node tests.

@m-e-l-u-h-a-n @timabbott
minor: Specify types of pills while initializing pill typeahead.
0080daa 
In options that we pass to pill_typeahead.set_up we
specify if we want typeahead to support stream or
user_group pills, and use users as source by default.

Using users for source by default, can have unnecessary
suggestions in typeaheads where only user_groups or streams
are needed.

So to solve that, we specify if we want users pill in the input.
This is then utilized in further commits, to clean up hacky code
that deals with intializing source for typeahead.
@m-e-l-u-h-a-n @timabbott
pills: Cleanup source initialization in pill_typeahead.
069718c 
Source in pill_typeahead, was intialized in a
very tricky way. In this cleanup, we refactor it
to clearly reflect how source is initialized for
different cases. These changes do not change its
behavior for its current use and solve potential
issues, so that it could be safely used at places
that do not require user pills at all.
@m-e-l-u-h-a-n @timabbott
typeahead: Make typeahead_helper.sort_recipients more readable.
22b1f6e
@timabbott
Copy link
Sponsor Member

timabbott commented May 25, 2021 •
edited
Loading

@m-e-l-u-h-a-n you may want to setup a spellchecker that runs when writing commit messages; you had a few typos like "redable" and "intialization" that I needed to manually correct.

I've merged this, but I think there's still more cleanup to do here:

  • The user_source local variable seems unnecessary; we could just access opts.user_source instead.
  • The highlighter and matcher functions could probably benefit from sharing structure with the source function to check include_user, etc.

@timabbott
Copy link
Sponsor Member

@m-e-l-u-h-a-n you may want to setup a spellchecker that runs when writing commit messages; you had a few typos like "redable" and "intialization" that I needed to manually correct.

I think there's still more cleanup to do here:

  • The user_source local variable seems unnecessary; we could just access opts.user_source instead.
  • The highlighter and matcher functions could probably benefit from sharing structure with the source function to check include_user, etc.

@timabbott timabbott closed this May 25, 2021
@m-e-l-u-h-a-n
Copy link
Member Author

Ohh, I'll look into this spelling problem next time I write any commit message.

@m-e-l-u-h-a-n m-e-l-u-h-a-n deleted the pill_typeahead_cleanup branch May 25, 2021 19:58
@m-e-l-u-h-a-n m-e-l-u-h-a-n mentioned this pull request May 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
size: XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@m-e-l-u-h-a-n @timabbott @zulipbot