zulip icon indicating copy to clipboard operation
zulip copied to clipboard

popover: Fix user pop over not showing when clicked in stream/group settings

Open PieterCK opened this issue 10 months ago • 3 comments

:bookmark_tabs: Overview

This PR aims to pick up where #28820 has left off. To avoid redundancy, please check the original PR thread for screenshots and demonstration of the changes introduced from this PR. Any newer updates and screenshots will be posted here. So far, the difference in this PR and the original is only small changes from addressing reviewers' comments.

Fixes: #28687

Screenshots and screen captures:

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.

PieterCK avatar Apr 30 '24 14:04 PieterCK

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

zulipbot avatar Apr 30 '24 14:04 zulipbot

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 :sweat_smile:

PieterCK avatar Apr 30 '24 15:04 PieterCK

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

alya avatar Apr 30 '24 20:04 alya

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 avatar May 04 '24 08:05 PieterCK

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! :+1:

PieterCK avatar May 04 '24 08:05 PieterCK

Awesome, thanks! @sahil839 could you please take a look next?

I can manually test too once you're happy with the code.

alya avatar May 06 '24 22:05 alya

Update: Addressed reviews

Posted few comments. I think you would want to update the commit messages to mention yourself as co-author, as the commits are still authored by @ecxtacy.

Ah, thanks for pointing it out. I've fixed the commit description and addressed your reviews. Thanks for reviewing

PieterCK avatar May 09 '24 08:05 PieterCK

Update: Addressed reviews.

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

PieterCK avatar May 10 '24 13:05 PieterCK

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.

sbansal1999 avatar May 10 '24 15:05 sbansal1999

Aha, all good. Thanks!

PieterCK avatar May 11 '24 11:05 PieterCK

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 avatar May 13 '24 03:05 sahil839

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.

sahil839 avatar May 14 '24 11:05 sahil839

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

alya avatar May 14 '24 20:05 alya

Resolved merge conflict. @alya this is ready for testing

PieterCK avatar May 17 '24 09:05 PieterCK

Works for me in manual testing!

alya avatar May 17 '24 16:05 alya

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

alya avatar May 17 '24 16:05 alya

Merged, thanks @PieterCK and @sahil839!

timabbott avatar May 17 '24 17:05 timabbott