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

Web: Use Intl.ListFormat rather than .join(", ") #28453

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

Conversation

CIC4DA
Copy link
Collaborator

@CIC4DA CIC4DA commented Jan 5, 2024

This PR brings internationalization by replacing the inconsistent .join(", ") with Intl.ListFormat for accurate and localized list formatting, using a common utility function.

Screenshot of Visual Changes:
visual_changes_direct_message

Edit : Removed Internationalization from left side bar.
image

  • Files which were to be internationalized are using Intl.ListFormat.
  • Uses { style: "long", type: "conjunction" } could be changed according to use.
  • Add a fallback system for MacOs.
  • Refactor tests to test with the new changes.

Fixes: #26936 Use Intl.ListFormat rather than .join(", ")

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
Copy link
Member

zulipbot commented Jan 5, 2024

Hello @CIC4DA, it seems like you have referenced #26936 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #26936..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

web/src/util.ts Outdated Show resolved Hide resolved
web/src/filter.ts Outdated Show resolved Hide resolved
@@ -132,7 +132,7 @@ test("process_new_message", () => {

assert.equal(message.reply_to, "bob@example.com,cindy@example.com");
assert.equal(message.to_user_ids, "103,104");
assert.equal(message.display_reply_to, "Bob, Cindy");
assert.equal(message.display_reply_to, "Bob and Cindy");
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

The web/src/message_notifications.js code path appears to use this field for non-display purposes to construct unique keys; I think that might now be incorrect

The web/src/recent_view_ui.js code also seems to sort by message.display_reply_to; does that seem correct to you? Might require some auditing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In web/src/message_notifications.js, we can convert back the internationalized list to an array and then to a list using .join(", "), and then set the key.

Something like this :

const reconstructed_list = message.display_reply_to
                          .replace('and', '') 
                          .split(',')
                          .map(item => item.trim())
                          .join(", ");

key = reconstructedList;

I'm not sure about web/src/recent_view_ui.js, as it uses message.display_reply_to for sorting and message.display_reply_to are already sorted name, with an extra and in it. Will it effect the sorting of multiple strings?
What is your view about it?

@timabbott
Copy link
Sponsor Member

@CIC4DA thanks for working on this; it looks pretty promising. I think what we'll need to merge this are:

  • Addressing the above feedback.
  • Making sure that test-js-with-node passes after each commit -- it looks like some of the test changes are in the wrong commit as noted in comments above, and this would prevent me from merging only the parts of this PR that I can approve to simplify further review.
  • Posting screenshots of the visual changes for review, at least for the majority of commits where only one place changes, so we can discuss if in any cases we want different ListFormat settings in any of them.

I expect to be able to merge most of the commits after that; the display_reply_to one might be worth just dropping from this and doing in a separate follow-up PR, just because it seems like it might touch a lot of UI/logic.

@CIC4DA CIC4DA requested a review from timabbott January 7, 2024 08:58
@CIC4DA CIC4DA force-pushed the internationalization branch 4 times, most recently from 11f2c26 to 0898321 Compare January 8, 2024 16:24
@CIC4DA
Copy link
Collaborator Author

CIC4DA commented Jan 8, 2024

@timabbott, I have done the following:

  • Addressed the feedback.
  • Each test change is included in the commit of file which caused the change.
  • Added a screenshot of the visual changes.
  • Removed message_store.display_replay_to commit from this PR

@timabbott
Copy link
Sponsor Member

I extracted several commits to #28611 to merge those; posting comments on the rest.

web/src/input_pill.ts Outdated Show resolved Hide resolved
@CIC4DA CIC4DA force-pushed the internationalization branch 9 times, most recently from fd883bc to 8f6c6be Compare January 29, 2024 09:52
@CIC4DA CIC4DA force-pushed the internationalization branch 2 times, most recently from 85c6475 to 8f6c6be Compare January 29, 2024 10:43
@CIC4DA CIC4DA requested a review from timabbott January 29, 2024 11:10
@CIC4DA
Copy link
Collaborator Author

CIC4DA commented Feb 1, 2024

@timabbott I have made some required changes, and requires suggestion from you on above reviews, kindly take a look. Thanks.

@CIC4DA CIC4DA force-pushed the internationalization branch 3 times, most recently from a4d486c to a80e29c Compare February 2, 2024 13:37
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The function get_cleaned_pm_recipients of typehead_helper.ts, was only used in .on(blur) function. But I didn't removed it as it can be used further in future. And to cover it in test, Added a test for get_cleaned_pm_recipients in typehead_helper.test.js

@zulipbot zulipbot added size: L and removed size: XL labels Feb 9, 2024
@timabbott timabbott added the integration review Added by maintainers when a PR may be ready for integration. label Feb 13, 2024
This commit removes the stale code for .on(blur) which was overwritten by input pill code. It includes the following changes:
- Removing the .on(blur) function
- Refactored composebox_typehead.test.js tests to comply with the change
@zulipbot
Copy link
Member

Heads up @CIC4DA, 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
has conflicts integration review Added by maintainers when a PR may be ready for integration. size: L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Intl.ListFormat rather than .join(", ")
5 participants