Replace Checkboxes List with Stream Pills in Invite Users Modal #26871 #28686

sanchi-t · 2024-01-23T19:32:12Z

The Invite Users Modal previously featured a list of checkboxes for selecting streams when inviting users. This has been replaced with an input field featuring typeahead and stream pills for streamlined stream selection.

Screenshots and screen captures:

Before:

After:

Dark mode:

Light Mode:

Self-review checklist

Self-reviewed the changes for clarity and maintainability
(variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

Explains differences from previous plans (e.g., issue description).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
Responsiveness and internationalization.
Strings and tooltips.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

sanchi-t · 2024-01-23T19:36:40Z

web/src/stream_pill.ts

+export function append_stream(
+    stream: StreamSubscription,
+    pill_widget: StreamPillWidget,
+    display_value: (stream: StreamSubscription) => string,
+): void {


Is using a parameter, show_stream_sub_size, a better approach than passing a function?

The show_stream_sub_size parameter approach has been employed in a similar PR, #24580.

web/src/add_streams_pill.ts

tools/test-js-with-node

timabbott · 2024-02-13T22:38:00Z

I guess this is still blocked on migrating pill_typeahead.js to TypeScript? I think there's not much left to do that, as detailed in #frontend.

timabbott · 2024-04-20T19:35:00Z

@sanchi-t the pill TypeScript work finally got integrated, so it should be possible to rebase and merge this one now!

shubham-padia

Looks good visually, thanks @sanchi-t ! I've left a few comments, it also might be worth investing some time into making the commit structure of this PR a bit better

web/src/stream_pill.ts

web/src/pill_typeahead.ts

web/src/add_streams_pill.ts

web/templates/invite_user_modal.hbs

shubham-padia

Left some comments. Moreover, the last 3 commits should be squashed since the css and test changes of a feature should be in the same commit.

web/src/add_streams_pill.ts

shubham-padia · 2024-06-03T02:25:43Z

web/src/pill_typeahead.ts

+        display_pill = stream_pill.display_pill;
+    } else {
+        display_pill = opts.display_pill;
+    }


This might be better framed as follows:

let display_pill: (sub: StreamSubscription) => string = stream_pill.display_pill; if (opts.display_pill) { display_pill = opts.display_pill; }

This makes it easier to read that stream_pill.display_pill is the default display_pill function and we use the opts.display_pill if it exists

I'm not entirely sure this version is easier to read; probably the ?? operator might be clearest?

So also of note is that this function being patched here is a generic function not limited to just streams, and yet we have a function called display_pill that is only relevant for those? That doesn't make sense.

Do we need a default value at all?

Do we need a default value at all?

No, we use display_pill when we call stream_pill.append. We can change the stream_pill.append function such that it accepts an optional parameter for getting stream pill label and it would provide the default value.

So also of note is that this function being patched here is a generic function not limited to just streams, and yet we have a function called display_pill that is only relevant for those? That doesn't make sense.

We can rename the function so it indicates its stream specific like stream_display_pill?

I think we need this function in pill_typeahead.ts because pill_typeahead calls stream_pill.append and we need to mention the display value of pill there. It wouldn't been have a problem if we displayed the same format value for stream pill everywhere, but for user invite modal we just want stream name as the display value and not extra information about subscriber count.

Also you suggested to have stream_match_prefix as a parameter which is also specific to streams so i think a function specific to streams should also be alright.

web/src/add_streams_pill.ts

web/src/pill_typeahead.ts

sahil839

Posted some comments.

Also, the invite modal is not opened if the user does not have permission to susbcribe others, so that should be fixed.

sahil839 · 2024-06-20T06:23:04Z

web/src/pill_typeahead.ts

        user_source?: () => User[];
        exclude_bots?: boolean;
        update_func?: () => void;
+        help_on_empty_strings?: boolean;


Are these changes required now since set_up_streams function is added?

Yeah, I am not sure. I thought Tim wanted that stream_match_prefix property for opts in set_up_combined.
Anyway I removed that commit.

web/src/stream_pill.ts

web/src/invite_stream_picker_pill.ts

web/src/pill_typeahead.ts

web/src/stream_data.ts

sahil839 · 2024-06-20T07:30:56Z

Also, I guess the previous approach that you used for translating the text inside the pills [here](#28686 (comment) was fine since variable values are not translated. You can see we use similar methods when text contains user on stream name, one is where we translate the "Already subscribed to {channel}" string in stream_settings_components.js.

sanchi-t · 2024-06-20T17:31:52Z

Also, the invite modal is not opened if the user does not have permission to susbcribe others, so that should be fixed.

What do you mean? If user does not have permission to subscribe other the invite modal should not be opened and that is the current behavior. I am not getting what you mean.

sanchi-t · 2024-06-20T17:37:37Z

Also, I guess the previous approach that you used for translating the text inside the pills [here](#28686 (comment) was fine since variable values are not translated. You can see we use similar methods when text contains user on stream name, one is where we translate the "Already subscribed to {channel}" string in stream_settings_components.js.

So what do you suggest? Use the previous approach? I think the current approach is better as it is easier/simpler to use stream_pill.create_item_from_stream_name.

sahil839 · 2024-06-21T07:16:42Z

Also, the invite modal is not opened if the user does not have permission to susbcribe others, so that should be fixed.

What do you mean? If user does not have permission to subscribe other the invite modal should not be opened and that is the current behavior. I am not getting what you mean.

It should open and the current behavior is that it does not open.

web/src/stream_pill.ts

web/tests/stream_data.test.js

web/src/stream_pill.ts

sahil839 · 2024-06-21T07:43:22Z

So what do you suggest? Use the previous approach? I think the current approach is better as it is easier/simpler to use stream_pill.create_item_from_stream_name

I am not sure. create_item_from_stream_name already accepts show_subscriber_count parameter, so should be easy to just compute the string there itself. It will also help in avoiding adding two extra parameters to StreamPill and other types.

sanchi-t · 2024-06-22T08:43:48Z

Updates since last review:

Used $t for translation (previous approach) instead of adding new properties to StreamPill.
The invite modal now opens if user does not have permission to subscribe other.
Addressed the review comments.

Use `$t` to ensure the function returns a translatable string.

timabbott · 2024-06-23T02:26:48Z

web/tests/pill_typeahead.test.js

+        assert.equal(typeof config.highlighter_html, "function");
+        assert.equal(typeof config.matcher, "function");
+        assert.equal(typeof config.sorter, "function");
+        assert.equal(typeof config.updater, "function");


I'm not really a fan of the testing strategy of doing so much mocking... It's a lot of test code for pretty minimal value.

I guess I can merge this version, but maybe we should discuss in #frontend what would be a better way to unit-test this.

timabbott · 2024-06-23T02:28:34Z

web/src/stream_pill.ts

@@ -67,33 +77,43 @@ export function get_user_ids(pill_widget: StreamPillWidget | CombinedPillContain

 export function append_stream(
    stream: StreamSubscription,
-    pill_widget: CombinedPillContainer,
+    pill_widget: StreamPillWidget | CombinedPillContainer,
+    show_subscriber_count = true,


It would be better to do the refactors to add options like this in their own commits, for future reference, since that helps a reviewer model how much risk there is of breaking other pill typeahead components.

Introduce an input field with typeahead functionality, initially populated with the default streams for the organization. Fixes zulip#26871.

timabbott · 2024-06-23T02:40:28Z

So I did a fairly extensive review, and I think this is basically good to merge. I ended up squashing the last two commits together, since that seemed more readable to me; the new final commit essentially fixed a correctness issue in the previous one. I found one bug, which I'll open as a follow-up issue that should be a high priority -- I don't intend to deploy this to production until it's fixed:

After you backspace to remove a stream pill, the pill typeahead opens, preventing you from using backspace to remove a second stream pill. Edit: Opened as Improve invitation modal stream pills backspace interaction #30542.

zulipbot added the size: XL label Jan 23, 2024

sanchi-t commented Jan 23, 2024

View reviewed changes

web/src/add_streams_pill.ts Outdated Show resolved Hide resolved

sanchi-t force-pushed the issue-26871 branch from 7496e7d to 24b83c0 Compare January 24, 2024 13:09

sanchi-t commented Jan 24, 2024

View reviewed changes

tools/test-js-with-node Outdated Show resolved Hide resolved

sanchi-t force-pushed the issue-26871 branch 3 times, most recently from a192317 to ade9654 Compare January 25, 2024 09:14

zulipbot added the has conflicts label Jan 26, 2024

sanchi-t force-pushed the issue-26871 branch from ade9654 to 6c755cd Compare January 28, 2024 06:49

zulipbot removed the has conflicts label Jan 28, 2024

zulipbot added the has conflicts label Feb 21, 2024

sanchi-t force-pushed the issue-26871 branch 4 times, most recently from 36a62d8 to cd5d1df Compare May 24, 2024 18:56

zulipbot removed the has conflicts label May 24, 2024

sanchi-t force-pushed the issue-26871 branch 2 times, most recently from 239e6bf to 28d6746 Compare May 24, 2024 20:37

sanchi-t requested a review from timabbott May 24, 2024 21:04

zulipbot added the has conflicts label May 26, 2024

sanchi-t force-pushed the issue-26871 branch from 28d6746 to 55ce50a Compare May 29, 2024 07:44

zulipbot removed the has conflicts label May 29, 2024

sanchi-t force-pushed the issue-26871 branch from 55ce50a to 71a9395 Compare May 29, 2024 13:42

shubham-padia reviewed May 31, 2024

View reviewed changes

sanchi-t force-pushed the issue-26871 branch from 71a9395 to fd6e7b4 Compare June 2, 2024 08:08

shubham-padia suggested changes Jun 3, 2024

View reviewed changes

sanchi-t force-pushed the issue-26871 branch from fd6e7b4 to f76e79b Compare June 4, 2024 10:46

sahil839 reviewed Jun 20, 2024

View reviewed changes

sanchi-t force-pushed the issue-26871 branch 3 times, most recently from 431a244 to ac515ad Compare June 20, 2024 17:26

sanchi-t force-pushed the issue-26871 branch from ac515ad to b1ccc8c Compare June 20, 2024 17:44

sanchi-t requested a review from sahil839 June 20, 2024 18:09

sahil839 reviewed Jun 21, 2024

View reviewed changes

web/src/stream_pill.ts Outdated Show resolved Hide resolved

sahil839 reviewed Jun 21, 2024

View reviewed changes

web/tests/stream_data.test.js Show resolved Hide resolved

sahil839 reviewed Jun 21, 2024

View reviewed changes

web/src/stream_pill.ts Outdated Show resolved Hide resolved

sanchi-t force-pushed the issue-26871 branch from b1ccc8c to cf0fd1c Compare June 22, 2024 08:08

sanchi-t requested a review from sahil839 June 22, 2024 08:41

sanchi-t added 2 commits June 22, 2024 18:06

stream_pill: Rename stream_display_pill.

1bc5505

stream_pill: Update format_stream_name_and_subscriber_count.

8b82b64

Use `$t` to ensure the function returns a translatable string.

timabbott reviewed Jun 23, 2024

View reviewed changes

invite: Replace stream checkboxes list with stream pills.

b3a1a29

Introduce an input field with typeahead functionality, initially populated with the default streams for the organization. Fixes zulip#26871.

timabbott force-pushed the issue-26871 branch from cf0fd1c to b3a1a29 Compare June 23, 2024 02:39

timabbott enabled auto-merge (rebase) June 23, 2024 02:40

timabbott mentioned this pull request Jun 23, 2024

Improve invitation modal stream pills backspace interaction #30542

Closed

timabbott merged commit 2d79ce2 into zulip:main Jun 23, 2024
14 checks passed

alya mentioned this pull request Jun 25, 2024

Improve channels typeahead on invite modal #30577

Closed

2 tasks

This was referenced Jun 25, 2024

Improve invitation modal stream pills backspace interaction #30542 #30578

Merged

Improve channels typeahead on invite modal #30577 #30580

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Checkboxes List with Stream Pills in Invite Users Modal #26871 #28686

Replace Checkboxes List with Stream Pills in Invite Users Modal #26871 #28686

sanchi-t commented Jan 23, 2024 •

edited

Loading

sanchi-t Jan 23, 2024

timabbott commented Feb 13, 2024

timabbott commented Apr 20, 2024

shubham-padia left a comment

shubham-padia left a comment

shubham-padia Jun 3, 2024

sanchi-t Jun 4, 2024

timabbott Jun 7, 2024

timabbott Jun 7, 2024

timabbott Jun 7, 2024

sanchi-t Jun 8, 2024

sanchi-t Jun 8, 2024

sahil839 left a comment

sahil839 Jun 20, 2024

sanchi-t Jun 20, 2024

sahil839 commented Jun 20, 2024

sanchi-t commented Jun 20, 2024

sanchi-t commented Jun 20, 2024

sahil839 commented Jun 21, 2024 •

edited

Loading

sahil839 commented Jun 21, 2024

sanchi-t commented Jun 22, 2024

timabbott Jun 23, 2024 •

edited

Loading

timabbott Jun 23, 2024

timabbott commented Jun 23, 2024 •

edited

Loading

Replace Checkboxes List with Stream Pills in Invite Users Modal #26871 #28686

Replace Checkboxes List with Stream Pills in Invite Users Modal #26871 #28686

Conversation

sanchi-t commented Jan 23, 2024 • edited Loading

Before:

After:

Dark mode:

Light Mode:

Choose a reason for hiding this comment

timabbott commented Feb 13, 2024

timabbott commented Apr 20, 2024

shubham-padia left a comment

Choose a reason for hiding this comment

shubham-padia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil839 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil839 commented Jun 20, 2024

sanchi-t commented Jun 20, 2024

sanchi-t commented Jun 20, 2024

sahil839 commented Jun 21, 2024 • edited Loading

sahil839 commented Jun 21, 2024

sanchi-t commented Jun 22, 2024

timabbott Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Jun 23, 2024 • edited Loading

sanchi-t commented Jan 23, 2024 •

edited

Loading

sahil839 commented Jun 21, 2024 •

edited

Loading

timabbott Jun 23, 2024 •

edited

Loading

timabbott commented Jun 23, 2024 •

edited

Loading