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

deactivated_user: Show deactivated status for deactivated users. #32732

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

deactivated_user: Show deactivated status for deactivated users. #32732

wants to merge 1 commit into from

Conversation

shuklamaneesh23
Copy link
Collaborator

@shuklamaneesh23 shuklamaneesh23 commented Dec 16, 2024 •
edited
Loading

This pull request introduces the "deactivated" status for users and updates various parts of the codebase to support this new status.

Fixes: #26833 .

Screenshots and screen captures:

Dark Theme Light Theme
Before After
Before After
Before After
Before After
Before After
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 zulipbot added size: L area: compose (recipient pickers) Channel/DM recipient picker UI area: left sidebar (display) Left sidebar visual design area: message feed (UI) Buttons/UI directly in the message feed (not popovers, etc.) area: right-sidebar labels Dec 16, 2024
@zulipbot
Copy link
Member

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

@zulipbot zulipbot added size: M and removed size: L labels Dec 17, 2024
@shuklamaneesh23
Copy link
Collaborator Author

Hey @alya, could you please provide an initial review pass on the UI changes in this PR? I have a few questions:

  • Is the color of the deactivated symbol correct?
  • Does the placement of the slashed-circle-icon in the input-pill and user search pill look good to you?
  • Should I display User Deactivated in place of a deactivated user's actual name?

Thanks, and I look forward to hearing from you!

@shuklamaneesh23 shuklamaneesh23 marked this pull request as ready for review December 17, 2024 15:16
@alya
Copy link
Contributor

alya commented Dec 17, 2024

Thanks! Take a closer look at the design in the issue. We don't want to allow transparency behind the icon when super-imposing it on the user's avatar.

@alya
Copy link
Contributor

alya commented Dec 17, 2024

In addition, it shouldn't be sticking out of the overall pill.

@alya
Copy link
Contributor

alya commented Dec 17, 2024

Should I display User Deactivated in place of a deactivated user's actual name?

No, why would we do that?

@alya
Copy link
Contributor

alya commented Dec 17, 2024

The color should match the inactive user circle.

@zulipbot zulipbot added size: L and removed size: M labels Dec 17, 2024
@shuklamaneesh23 shuklamaneesh23 force-pushed the deactive branch 2 times, most recently from d18b292 to e175f14 Compare December 17, 2024 19:29
@shuklamaneesh23
Copy link
Collaborator Author

Hi @alya , I have adressed all the design suggestions you told above which includes:

  • To give color which should match the inactive user circle.
  • don't want to allow transparency behind the icon when super-imposing it on the user's avatar.
  • it shouldn't be sticking out of the overall pill.

Please take another look whenever you have time and apologies for my misunderstanding in implementing the design.
Thanks!

@alya
Copy link
Contributor

alya commented Dec 18, 2024

Please take a close look at the placement of the circles on the pills in the design. They are clearly not the placed the same way in your screenshots.

image

@zulipbot zulipbot added size: XL and removed size: L labels Dec 18, 2024
@shuklamaneesh23
Copy link
Collaborator Author

Hey @alya , I tried to best accompany the placement of circle again. I have updated the screenshots in the PR description.
Here, is a sample and more focussed one for the reference. Please have a look at it:

Screenshot 2024-12-18 at 12 54 56 PM

@alya
Copy link
Contributor

alya commented Dec 19, 2024

Thanks, that looks a lot better. I still see some avatar poking out on the bottom right on message senders.

Screenshot 2024-12-18 at 23 16 37

@alya
Copy link
Contributor

alya commented Dec 19, 2024

We should also add the circle to the user card for consistency.

Screenshot 2024-12-18 at 23 13 31

@alya
Copy link
Contributor

alya commented Dec 19, 2024

I think we also need a more detailed spec on the colors. I'll post on the design thread.

@shuklamaneesh23
Copy link
Collaborator Author

We don't want the pink outer circle to poke out the bottom here:

Hey @alya , I have updated the same to reflect this.
Please have another look.
Thanks!

@alya
Copy link
Contributor

alya commented Jan 4, 2025 •
edited
Loading

It's looking pretty good in the current UI, but we're working on #32616 , so it also needs to work at font sizes ranging from 12px to 20px. It currently looks like this at 20px (which you can configure in personal settings > preferences in dev):

Screenshot 2025-01-04 at 14 59 43@2x

If you are stuck on how to fix this, you can post a question in #frontend.

@shuklamaneesh23 shuklamaneesh23 force-pushed the deactive branch 2 times, most recently from b65b12b to 22d5127 Compare January 5, 2025 12:40
@shuklamaneesh23
Copy link
Collaborator Author

Hi @alya , I have updated the code to reflect the same UI at font sizes ranging from 12px to 20px.
Please have a look at them.
Here are some screenshots at various font-sizes for your reference.

At 12px: Screenshot 2025-01-05 at 6 07 24 PM
At 14px: Screenshot 2025-01-05 at 6 05 45 PM
At 16px: Screenshot 2025-01-05 at 6 06 22 PM
At 18px: Screenshot 2025-01-05 at 6 06 40 PM
At 20px: Screenshot 2025-01-05 at 6 06 57 PM

@alya
Copy link
Contributor

alya commented Jan 6, 2025

Cool, the deactivated marker in those screenshots looks good to me.

@alya
Copy link
Contributor

alya commented Jan 6, 2025

@evykassirer would you be up for reviewing this one?

@alya alya added the maintainer review PR is ready for review by Zulip maintainers. label Jan 6, 2025
evykassirer
evykassirer reviewed Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@evykassirer evykassirer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great! Clean and easy to review, and I like the way it looks in the app. I left a few comments -- let me know if you have any questions :)

web/src/presence.ts Outdated
@@ -60,8 +60,15 @@ export function get_status(user_id: number): PresenceStatus["status"] {
return "offline";
}
if (presence_info.has(user_id)) {
if (presence_info.get(user_id)!.status === undefined) {
return "offline";
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these three lines relevant for adding deactivated users? It doesn't seem like it would change that behavior. Was it an existing bug?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I can reach a state where I'm starting a message with a deactivated user (but can't send them anything) and their status circle in the DM window isn't the deactivated symbol:

image

And removing these three lines actually fixes this bug, so I think they should be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed these lines, but I still encounter this bug. This happens because the interface does not get updated live when a user is deactivated. You can observe similar behavior in the main branch.

Image Screenshot 2025-01-13 at 2 29 00 PM

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the first bug was worse because it would show up somewhere (compose box) but didn't in the sidebar.

But the thing you're sharing (not updating live when a user is deactivated) isn't great. This seems fine to fix as a followup, though let's check with @alya in case she wants it as part of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine by me, @timabbott can review and decide what makes sense.

web/src/buddy_data.ts Outdated Show resolved Hide resolved
web/templates/input_pill.hbs Outdated Show resolved Hide resolved
web/templates/search_user_pill.hbs Outdated Show resolved Hide resolved
web/styles/input_pill.css Outdated Show resolved Hide resolved
web/styles/input_pill.css Outdated Show resolved Hide resolved
@shuklamaneesh23 shuklamaneesh23 force-pushed the deactive branch 3 times, most recently from d8a7113 to fa5d1fd Compare January 13, 2025 10:15
@shuklamaneesh23
Copy link
Collaborator Author

Hi @evykassirer , thanks for the review.
I have made the changes as per the review.

@zulipbot zulipbot added size: L and removed size: XL labels Jan 13, 2025
@evykassirer
Copy link
Collaborator

Responded to two remaining open threads, but otherwise looks great! Thanks again for working on this :)

@shuklamaneesh23
Copy link
Collaborator Author

Hi @evykassirer, thanks for the review.

I have updated the one open thread and am waiting for @alya's thoughts on the other to determine how to proceed further.

@alya 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 Jan 22, 2025
@alya
Copy link
Contributor

alya commented Jan 22, 2025

@timabbott I think this is ready for you to take a look after @evykassirer 's review; let me know if you'd like me to re-test. Nice to have, but not specifically a release goal.

@evykassirer evykassirer assigned timabbott and unassigned evykassirer Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evykassirer evykassirer evykassirer left review comments

@alya alya alya left review comments

Assignees

@timabbott timabbott

Labels
area: compose (recipient pickers) Channel/DM recipient picker UI area: left sidebar (display) Left sidebar visual design area: message feed (UI) Buttons/UI directly in the message feed (not popovers, etc.) area: right-sidebar integration review Added by maintainers when a PR may be ready for integration. size: L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show "deactivated" status for deactivated users
5 participants
@shuklamaneesh23 @zulipbot @alya @evykassirer @timabbott