zulip
zulip copied to clipboard
popover: Fix user pop over not showing when clicked in stream/group settings
: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.
Hello @zulip/server-settings members, this pull request was labeled with the "area: settings UI" label, so you may want to check it out!
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:
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.
Update: Edited the PR description, added new changes to the PR addressing reviews from the original PR and highlighted those changes through PR comments.
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:
Awesome, thanks! @sahil839 could you please take a look next?
I can manually test too once you're happy with the code.
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
Update: Addressed reviews.
Having trouble trying to resolve the failing tests though, looks like it failed during provisioning. Any advice? @sbansal1999 @sahil839
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.
Aha, all good. Thanks!
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.
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.
@PieterCK Please bump for my review once conflicts are resolved.
Resolved merge conflict. @alya this is ready for testing
Works for me in manual testing!
@timabbott This PR addresses @sahil839 's feedback on an earlier PR.
Merged, thanks @PieterCK and @sahil839!