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.

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

Fix: order of group direct message recipients. #28738

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

richardshaju
Copy link
Collaborator

Fix: order of group direct message recipients
Fixes: #27375

Screenshots and screen captures:
before :
Screenshot 2024-01-28 225928

after:
Screenshot 2024-01-28 230111

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

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: S area: compose (misc) area: compose (recipient pickers) Stream/DM recipient picker UI area: left sidebar (display) Left sidebar visual design area: message feed (display) Web frontend rendering of messages (post markdown) labels Jan 29, 2024
web/src/people.ts Outdated Show resolved Hide resolved
@richardshaju
Copy link
Collaborator Author

richardshaju commented Feb 2, 2024

Changes made as per the review.

@timabbott
Copy link
Sponsor Member

Can you explain how this completes the issue? It talks about several locations and the issue of case-insensitive ordering, and I'd like to see a complete audit confirming this results in the intended logic in all places listed.

@richardshaju
Copy link
Collaborator Author

I apologize for any inconvenience caused. I will take the necessary steps to make the required changes.

@zulipbot zulipbot added size: M and removed size: S labels Feb 8, 2024
@richardshaju richardshaju force-pushed the fix/sort-dm-recipients branch 2 times, most recently from a988ccc to 30de795 Compare February 8, 2024 06:30
@richardshaju
Copy link
Collaborator Author

Before:

After:

@timabbott
Copy link
Sponsor Member

@richardshaju can you fix the test failures?

I appreciate the screenshots, but I'd also appreciate a paragraph or so write-up explaining the analysis for how you determined this is a complete and correct solution.

@timabbott
Copy link
Sponsor Member

Also it'd be helpful if you discuss how you've addressed the feedback on #27557, a previous PR for this feature.

@richardshaju richardshaju force-pushed the fix/sort-dm-recipients branch 3 times, most recently from e7ed63e to 7ce639d Compare February 15, 2024 06:27
@richardshaju
Copy link
Collaborator Author

  • As mentioned in the issue, the DMs list in the left sidebar can be sorted by using the util.make_strcmp() in the function get_recipients() in people.ts.

Screenshot 2024-02-15 114702

  • The order of the placeholder shown below is fixed by the same approach as the above. This time at get_pm_full_names() in message_store.ts.

Screenshot 2024-02-15 114724

  • This order is fixed again through the same approach at compare_by_name in message_list_view.js.

Screenshot 2024-02-15 103457


All the order of names given below including the pills are fixed using the new function sort_emails() in people.ts. This new function sorts the emails based on their full names. This new function is used in user_ids_string_to_emails_string() and pm_reply_to() both are from people.ts.

  • The given below are fixed by the user_ids_string_to_emails_string() by using the sort_emails()

Screenshot 2024-02-15 114824

Screenshot 2024-02-15 114755

  • The given below problem is fixed by the pm_reply_to() by using the sort_emails()
aaron.Cordelia.Lear.s.daughter.Zoe.-.Zulip.Dev.-.Zulip.-.Google.Chrome.2024-02-15.10-44-16.mp4

@richardshaju
Copy link
Collaborator Author

I appreciate @Jenil-Dobariya for PR #27557. It helped me a lot, especially in writing the tests. I used the same approach with some optimization mentioned in the code review. I think he is nearly finished with the PR work, but unfortunately, he hasn't been responding.

@richardshaju
Copy link
Collaborator Author

@timabbott Please review this.

@pratik-pc
Copy link
Collaborator

Hey @richardshaju, I haven't really gone through the code but have found some inconsistencies. It seems it doesn't sort correctly when narrow triggered from message header.

Screencast.from.07-03-24.12.42.26.AM.IST.webm

@richardshaju
Copy link
Collaborator Author

@pratik-pc Thank you for figuring this out.

@richardshaju
Copy link
Collaborator Author

richardshaju commented Mar 11, 2024

@pratik-pc Is this the correct approach?

All.messages.-.Zulip.Dev.-.Zulip.-.Google.Chrome.2024-03-11.15-22-14.mp4

@pratik-pc
Copy link
Collaborator

@richardshaju Some areas are still left

Screencast.from.12-03-24.12.33.10.AM.IST.webm

@richardshaju
Copy link
Collaborator Author

@pratik-pc But in my system it's working properly.

aaron.Desdemona.King.Hamlet.Zoe.-.Zulip.Dev.-.Zulip.-.Google.Chrome.2024-03-13.23-44-05.mp4

@pratik-pc
Copy link
Collaborator

pratik-pc commented Mar 13, 2024

Hey @richardshaju, steps to repro are:

  • Save to draft with unsorted recipient list
  • Then narrow from drafts

Also

  • Narrow to group convo from compose box with unsorted recipient list

@richardshaju
Copy link
Collaborator Author

@pratik-pc but now I can't save the draft with the unsorted recipient list.

@pratik-pc
Copy link
Collaborator

@richardshaju Here's how it is for me

Screencast.from.15-03-24.11.43.29.PM.IST.webm

@zulipbot zulipbot added size: L and removed size: M labels Mar 25, 2024
@timabbott timabbott added the completion candidate PRs with reviews that may unblock merging label Apr 14, 2024
Fixed the order of recipients in search bar title.
Compose box placeholder and recipient pill input.
Created a new function sort_emails in people.ts.
Fixes zulip#27375.
@zulipbot
Copy link
Member

Heads up @richardshaju, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: compose (misc) area: compose (recipient pickers) Stream/DM recipient picker UI area: left sidebar (display) Left sidebar visual design area: message feed (display) Web frontend rendering of messages (post markdown) completion candidate PRs with reviews that may unblock merging has conflicts size: L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix order of group direct message recipients.
4 participants