Skip to content

Conversation

neiljp
Copy link
Collaborator

@neiljp neiljp commented Apr 12, 2023

What does this PR do, and why?

Following a refactor commit that I created incidentally when looking at #1376, I realized that #1187 would be simpler to rebase/refactor/merge, and this is the result. This also introduces a Literal type for the valid status values we use internally.

As per the intent of #1187, this uses a dedicated marker for bots in the user list and wherever else their status appears, such as next to their name in any messages from them (direct messages, stream messages).

Note the marker is not per the initial screenshot of #1187, since we need a simple character that is visible across more fonts/terminals (pending #938), as discussed in #1187 and in the stream.

Outstanding aspect(s)

  • UserButton takes a parameter of the state_marker, so passing in a bot user and expecting a different output is not testable right now

External discussion & connections

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

See #1187 and the linked topic.

neiljp and others added 5 commits April 12, 2023 13:36
This enables the STATE_ICON lookup to be typed more cleanly.
This significantly reduces code duplication.

Note added in ui_mappings that the order is relevant in the UI.
Bots do not have a dynamic status, so give them a static one.
These are constructed dynamically based on the user status, so if a user may
have a "bot" status, a "user_bot" style is now necessary.
This will be picked up using earlier commits, to show it differently in
the UI.

With a different status, bots are now explicitly placed at the end of the
user list.

Tests updated.

Original logic by Progyan, simplified using logic suggested in review by
neiljp in zulip#1187.

Co-authored-by: neiljp (Neil Pilgrim) <[email protected]>
@neiljp neiljp added the area: UI General user interface update label Apr 12, 2023
@neiljp neiljp added this to the Next Release milestone Apr 12, 2023
@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Apr 12, 2023
@neiljp neiljp added the PR ready to be merged PR has been reviewed & is ready to be merged label Apr 12, 2023
@neiljp neiljp merged commit 74bcb11 into zulip:main Apr 12, 2023
@neiljp neiljp mentioned this pull request Apr 12, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: UI General user interface update PR ready to be merged PR has been reviewed & is ready to be merged size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants