popover: Fix user pop over not showing when clicked in channels /group settings #29898

PieterCK · 2024-04-30T14:56:12Z

📑 Overview

This PR aims to continue where #28820 has left off. The changes introduced in my PR are primarily fixes addressing the comments left by reviewer in the original PR.

Details of the original PR by Ecxtacy:

Added css fix for hovering effect on the X icon in user pills.
Added an optimization refactor to removePill function of input_pill.ts module as discussed in the CZO.
Fixed user pill related components to enable popover effect for user and user group pills in channel and group settings.

New changes in this PR is detailed in the PR comments I've highlighted in the code.

Fixes: #28687

Screenshots and screen captures:

Popovers on user and group pills in channel and group menu:

Relevant commits for this change:

input_pill: Pass user_id and group_id to render pill.
settings: Show user_card_popover when pill is clicked.
popovers: Refactor user_group_popover to work with pills.
settings: Show user_group_popover when pill is clicked.

Popovers on Channel/Stream settings:

Before	After

Popovers on User Groups Settings:

Before	After

X button hover effect:

Relevant commit for this change: css: Show 'x' button hover styling when hovered over 'x'.

Before:
X button focuses even only by hovering over the pill and not the button itself.

Screencast.from.04-05-24.16.06.44.webm

After:
X button only focuses if the cursor is on top of it.

Screencast.from.04-05-24.14.38.21.webm

Popover closing behavior:

Relevant commit for this change: hashchange: Fix popover not disappearing on overlay hash URL.

Before:
Clicking back button in browser doesn't hide popovers

Screencast.from.04-05-24.15.40.48.webm

After:

Screencast.from.04-05-24.15.57.19.webm

Screencast.from.04-05-24.15.57.53.webm

Impacted functions, no regression found:

toggle_user_group_info_popover() in user_group_popover.js still function on mentions in messages:

other user pills in group & channel setting:

Screencast.from.03-05-24.19.58.05.webm

🧑‍🏭 Concerns & Issue Encountered

inconsistency in user_group_pill.ts. I've pointed out the detail here

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.

zulipbot · 2024-04-30T14:56:20Z

Hello @zulip/server-settings members, this pull request was labeled with the "area: settings UI" label, so you may want to check it out!

PieterCK · 2024-04-30T15:08:09Z

umm, hello @ecxtacy. I think I did an oopsie by mentioning(@) you in the PR commits. I think you'll get notifications eveytime I push something here. I'm not sure whether that stays true even if I've changed the commit descriptions to not mention you. Sorry for that, let me know if you need help turning it off 😅

alya · 2024-04-30T20:46:01Z

Thanks! To prepare this PR for review, you'll need to complete the self-review checklist, and reply to @sahil839 's comments on #28820, which had some questions. You can do so by commenting on this PR, including the original comment and your response.

PieterCK · 2024-05-03T08:10:55Z

web/src/input_pill.ts

@@ -19,6 +19,8 @@ export type InputPillItem<T> = {
 deactivated?: boolean;
 status_emoji_info?: EmojiRenderingDetails & {emoji_alt_code?: boolean}; // TODO: Move this in user_status.js
 should_add_guest_user_indicator?: boolean;
+ user_id?: number;
+ id?: number;


Original Review:

sahil: Why not group_id here instead of id?

Hi Sahil, it's because user_group_pill.ts used id instead if group_id. I also agree that it be group_id, consistent with user_id instead of just id. I can add it as a prep commit in this PR if you think we should make the change

relevant code in user_group_pill.ts:

Yes, I think group_id is better and you can do that change in a prep commit.

web/src/hashchange.js

PieterCK · 2024-05-03T08:15:25Z

web/templates/input_pill.hbs

@@ -1,4 +1,4 @@
-<div class='pill {{#if deactivated}} deactivated-pill {{/if}}' tabindex=0>
+<div class='pill {{#if deactivated}} deactivated-pill {{/if}}'{{#if user_id}}data-user-id="{{user_id}}"{{/if}}{{#if group_id}}data-group-id="{{group_id}}"{{/if}} tabindex=0>


Original Review:

sahil: I think we should name these attributes to data-user-id and data-group-id.

Changed it!

I guess we use data-user-group-id in user_group.toggle_user_group_info_popover, so we might use data-user-group-id here as well.

PieterCK · 2024-05-03T08:23:07Z

web/src/user_card_popover.js

- });
+ $("body").on(
+ "click",
+ ".view_user_profile, .pill-container.person_picker > .pill[data-user-id]",


In the original PR, there's a new separate function responsible for adding events listeners to the pill on stream and group settings. It was on line 885 in web/src/user_card_popover.js. However, it introduced a new bug where clicking some user pill pops up two popovers since it's targeted by both event listeners. So, I extended the selector to this event listener instead.

double popover bug:

I guess that was due to the selector being used in the original PR. I imagine in future we would just add a common class to all the user pills when we want to show the popover for all pills with proper testing.

This seems fine for now though. Or maybe you can just use .person_picker .pill[data-user-id] selector.

PieterCK · 2024-05-03T08:26:56Z

web/src/user_group_popover.ts

@@ -130,6 +140,17 @@ export function register_click_handlers(): void {
 blueslip.info("Unable to find user group in message" + message.sender_id);
 }
 });
+
+ // Show the user_group_popover when pill clicked in subscriber settings.
+ $("body").on("click", ".pill-container.person_picker > .pill[data-group-id]", (e) => {


Narrowed down the JQuery selector targeting the pill.
Original: .pill[group-id]

web/src/user_group_popover.ts

PieterCK · 2024-05-04T08:38:59Z

Update: Edited the PR description, added new changes to the PR addressing reviews from the original PR and highlighted those changes through PR comments.

PieterCK · 2024-05-04T08:40:15Z

Thanks! To prepare this PR for review, you'll need to complete the self-review checklist, and reply to @sahil839 's comments on #28820, which had some questions. You can do so by commenting on this PR, including the original comment and your response.

Sorry for notifying a sloppy PR, I've improved the PR description and commented on the new changes. I believe it's clearer now. Thank you as always! 👍

PieterCK · 2024-05-10T13:31:54Z

Update: Addressed reviews.

Having trouble trying to resolve the failing tests though, looks like it failed during provisioning. Any advice?
@sbansal1999 @sahil839

sbansal1999 · 2024-05-10T15:00:11Z

Yeah, looks like some issue during provisioning (can happen sometimes; nothing to worry about). You can re-run the tests by force-pushing your commits again.

Do it by amending your last commit's message using git commit --amend and then saving the commit message without making any changes. After that you can do a force push, which will trigger the tests again.

PieterCK · 2024-05-11T11:45:20Z

Aha, all good. Thanks!

sahil839 · 2024-05-13T03:51:48Z

Code looks good. @alya this is ready for testing.

A couple of points -

We still change the color of pill to green on clicking on them.
This PR also adds code to show the pills when clicking on pills in a person type custom profile field, like Mentor field in "Profile" panel or manage user form in development environment due to elements using same class, but I guess that should be fine.

sahil839 · 2024-05-14T11:27:54Z

Also, there is a PR #24334, which adds support for opening the popover for all the pills. This PR can be followed by completing that PR.

alya · 2024-05-14T20:22:26Z

@PieterCK Please bump for my review once conflicts are resolved.

Added new utility function to hash_parser to dynamically detect the hash category of an URL. This is intended to reduce the need of adding more adhoc hash_parser utility functions.

When input pills are hovered, the 'x' button is highlighted. This commit ensures that 'x' button is highlighted only if the 'x' button is hovered, not anywhere else on the pill. Author: ecxtacy <[email protected]>.

Changed UserGroupPill's 'id' key to 'group_id' because UserPill uses 'user_id' instead of only id.

While rendering input pills in input_pills.create function, pass in the user_id or group_id accordingly. Display the id as an html attribute in input_pill.hbs. Co-authored-by: Pieter CK <[email protected]>.

Add a 'click' event listener to the user pills in stream and group settings which toggles the user_card_popover. Co-authored-by: PieterCK <[email protected]>.

Refactored hashchanged() so that popover.hide_all() is executed before checking is_overlay_hash() to close popovers when the user changes hash but still on overlay.

Add 'click' event listener to the pills in stream settings to toggle the user_group_info_popover. Fixes zulip#28687. Co-authored-by: Pieter CK <[email protected]>.

The removePill function of input_pill module uses a for loop for iterating and finding the required pill. Replace the for loop with inbuilt findIndex function for efficiency. Authored-by: Dhruv Chouhan <[email protected]>.

PieterCK · 2024-05-17T09:55:03Z

Resolved merge conflict. @alya this is ready for testing

alya · 2024-05-17T16:53:56Z

Works for me in manual testing!

alya · 2024-05-17T16:54:34Z

@timabbott This PR addresses @sahil839 's feedback on an earlier PR.

timabbott · 2024-05-17T17:47:33Z

Merged, thanks @PieterCK and @sahil839!

zulipbot added size: L area: settings UI area: settings (user groups) User groups related settings issues priority: high labels Apr 30, 2024

PieterCK force-pushed the issue-28687-pill-click-popover branch 2 times, most recently from 718ebcd to dde88c0 Compare April 30, 2024 15:04

PieterCK marked this pull request as ready for review April 30, 2024 15:28

PieterCK force-pushed the issue-28687-pill-click-popover branch from dde88c0 to eebcc51 Compare May 3, 2024 03:24

PieterCK commented May 3, 2024

View reviewed changes

web/src/hashchange.js Show resolved Hide resolved

PieterCK commented May 3, 2024

View reviewed changes

PieterCK force-pushed the issue-28687-pill-click-popover branch 2 times, most recently from 2fe5403 to f293cd6 Compare May 3, 2024 12:39

PieterCK commented May 3, 2024

View reviewed changes

web/src/user_group_popover.ts Show resolved Hide resolved

PieterCK force-pushed the issue-28687-pill-click-popover branch from f293cd6 to b6557e9 Compare May 3, 2024 12:49

PieterCK marked this pull request as draft May 3, 2024 13:02

PieterCK force-pushed the issue-28687-pill-click-popover branch 3 times, most recently from 4f15dfb to 00caaf0 Compare May 4, 2024 04:05

PieterCK changed the title ~~popover: Fix user pop over not showing when clicked in stream/group settings~~ popover: Fix user pop over not showing when clicked in channels /group settings May 4, 2024

PieterCK force-pushed the issue-28687-pill-click-popover branch from 00caaf0 to 02a8d15 Compare May 4, 2024 08:23

PieterCK force-pushed the issue-28687-pill-click-popover branch 2 times, most recently from 6d5201c to 4e365c4 Compare May 4, 2024 09:24

PieterCK force-pushed the issue-28687-pill-click-popover branch 3 times, most recently from 4c72be7 to f8a7d51 Compare May 10, 2024 12:05

PieterCK force-pushed the issue-28687-pill-click-popover branch 2 times, most recently from 6c28e20 to 587d6d2 Compare May 11, 2024 05:54

zulipbot added the has conflicts label May 14, 2024

timabbott mentioned this pull request May 15, 2024

popovers: Show user and group popovers when pill clicked in stream and group settings. #28820

Closed

12 tasks

PieterCK and others added 8 commits May 17, 2024 15:58

hash_parser: Add utility function to dynamically detect hash.

d6a89b2

Added new utility function to hash_parser to dynamically detect the hash category of an URL. This is intended to reduce the need of adding more adhoc hash_parser utility functions.

css: Show 'x' button hover styling when hovered over 'x'.

49f0412

When input pills are hovered, the 'x' button is highlighted. This commit ensures that 'x' button is highlighted only if the 'x' button is hovered, not anywhere else on the pill. Author: ecxtacy <[email protected]>.

settings: Refactor UserGroupPill to improve consistency.

a3aa158

Changed UserGroupPill's 'id' key to 'group_id' because UserPill uses 'user_id' instead of only id.

input_pill: Pass user_id and group_id to render pill.

d3f213f

While rendering input pills in input_pills.create function, pass in the user_id or group_id accordingly. Display the id as an html attribute in input_pill.hbs. Co-authored-by: Pieter CK <[email protected]>.

settings: Show user_card_popover when pill is clicked.

ac0f5e9

Add a 'click' event listener to the user pills in stream and group settings which toggles the user_card_popover. Co-authored-by: PieterCK <[email protected]>.

hashchange: Fix popover not disappearing on overlay hash URL.

614da17

Refactored hashchanged() so that popover.hide_all() is executed before checking is_overlay_hash() to close popovers when the user changes hash but still on overlay.

settings: Show user_group_popover when pill is clicked.

19cb29d

Add 'click' event listener to the pills in stream settings to toggle the user_group_info_popover. Fixes zulip#28687. Co-authored-by: Pieter CK <[email protected]>.

pills: Optimize removePill function.

fc4a014

The removePill function of input_pill module uses a for loop for iterating and finding the required pill. Replace the for loop with inbuilt findIndex function for efficiency. Authored-by: Dhruv Chouhan <[email protected]>.

PieterCK force-pushed the issue-28687-pill-click-popover branch from 587d6d2 to fc4a014 Compare May 17, 2024 09:26

zulipbot removed the has conflicts label May 17, 2024

alya added integration review Added by maintainers when a PR may be ready for integration. and removed maintainer review PR is ready for review by Zulip maintainers. labels May 17, 2024

timabbott merged commit 6af748f into zulip:main May 17, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

popover: Fix user pop over not showing when clicked in channels /group settings #29898

popover: Fix user pop over not showing when clicked in channels /group settings #29898

PieterCK commented Apr 30, 2024 •

edited

zulipbot commented Apr 30, 2024

PieterCK commented Apr 30, 2024

alya commented Apr 30, 2024

PieterCK May 3, 2024 •

edited

sahil839 May 8, 2024

PieterCK May 3, 2024 •

edited

sahil839 May 8, 2024

PieterCK May 3, 2024 •

edited

sahil839 May 8, 2024

PieterCK May 3, 2024 •

edited

PieterCK commented May 4, 2024

PieterCK commented May 4, 2024 •

edited

PieterCK commented May 10, 2024

sbansal1999 commented May 10, 2024 •

edited

PieterCK commented May 11, 2024 •

edited

sahil839 commented May 13, 2024

sahil839 commented May 14, 2024

alya commented May 14, 2024

PieterCK commented May 17, 2024

alya commented May 17, 2024

alya commented May 17, 2024

timabbott commented May 17, 2024

		@@ -1,4 +1,4 @@
		<div class='pill {{#if deactivated}} deactivated-pill {{/if}}' tabindex=0>
		<div class='pill {{#if deactivated}} deactivated-pill {{/if}}'{{#if user_id}}data-user-id="{{user_id}}"{{/if}}{{#if group_id}}data-group-id="{{group_id}}"{{/if}} tabindex=0>

popover: Fix user pop over not showing when clicked in channels /group settings #29898

popover: Fix user pop over not showing when clicked in channels /group settings #29898

Conversation

PieterCK commented Apr 30, 2024 • edited

📑 Overview

🧑‍🏭 Concerns & Issue Encountered

zulipbot commented Apr 30, 2024

PieterCK commented Apr 30, 2024

alya commented Apr 30, 2024

PieterCK May 3, 2024 • edited

Choose a reason for hiding this comment

sahil839 May 8, 2024

Choose a reason for hiding this comment

PieterCK May 3, 2024 • edited

Choose a reason for hiding this comment

sahil839 May 8, 2024

Choose a reason for hiding this comment

PieterCK May 3, 2024 • edited

Choose a reason for hiding this comment

sahil839 May 8, 2024

Choose a reason for hiding this comment

PieterCK May 3, 2024 • edited

Choose a reason for hiding this comment

PieterCK commented May 4, 2024

PieterCK commented May 4, 2024 • edited

PieterCK commented May 10, 2024

sbansal1999 commented May 10, 2024 • edited

PieterCK commented May 11, 2024 • edited

sahil839 commented May 13, 2024

sahil839 commented May 14, 2024

alya commented May 14, 2024

PieterCK commented May 17, 2024

alya commented May 17, 2024

alya commented May 17, 2024

timabbott commented May 17, 2024

PieterCK commented Apr 30, 2024 •

edited

PieterCK May 3, 2024 •

edited

PieterCK May 3, 2024 •

edited

PieterCK May 3, 2024 •

edited

PieterCK May 3, 2024 •

edited

PieterCK commented May 4, 2024 •

edited

sbansal1999 commented May 10, 2024 •

edited

PieterCK commented May 11, 2024 •

edited