Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Change dash to em dash #8449

Closed
wants to merge 8 commits into from
Closed

Conversation

goelesha
Copy link
Contributor

@goelesha goelesha commented Apr 29, 2022

Fixes: element-hq/element-web#21895
Signed-off-by: Esha Goel goelesha2001@gmail.com


This PR currently has no changelog labels, so will not be included in changelogs.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

@goelesha goelesha requested a review from a team as a code owner April 29, 2022 20:01
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

This appears to only touch the one string, but I am sure we have more which could be em dashes. Ultimately though I think this might be something for Design to consider so they can be aware of it for future copy as well.

@@ -676,7 +676,7 @@
"Discovery": "الاكتشاف",
"Deactivate account": "تعطيل الحساب",
"Deactivate Account": "تعطيل الحساب",
"Deactivating your account is a permanent action - be careful!": "يعد تعطيل حسابك إجراءً دائمًا - كن حذرًا!",
"Deactivating your account is a permanent action —— ßbe careful!": "يعد تعطيل حسابك إجراءً دائمًا - كن حذرًا!",
Copy link
Member

Choose a reason for hiding this comment

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

translation files not modified by yarn i18n cannot be touched as it causes merge conflicts in the translation system. We'd ultimately have to fix them in the translations system instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Can you pls guide about how can this be done then

Copy link
Contributor

Choose a reason for hiding this comment

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

You should only edit the .tsx file, delete all the other changes and then run yarn i18n which will generate changes to the English translation file. The other translation files are then going to be modified through https://translate.element.io/

Copy link
Contributor

Choose a reason for hiding this comment

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

You should only edit the .tsx file, delete all the other changes and then run yarn i18n which will generate changes to the English translation file. The other translation files are then going to be modified through https://translate.element.io/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reversed the changes in .json files. Following the procedure suggested by @SimonBrandner after editing the .tsx file, and running yarn i18n I still can't see any changes in the en_EN.json file. It still looks like this-
Screenshot 2022-04-30 at 1 18 59 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

I can still see a bunch of changes JSON files in the diff - did you push the changes? Are you saying that if you change the dash in the .tsx file and then run yarn i18n nothing the en_EN.json file doesn't get updated with the new dash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed the changes. Still, I will check again.

Yes, actually I tried making the changes in a separate branch but nothing changes in the en_EN.json even after running yarn i18n.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed all the changes. It can be viewed from the latest commit here
20758ff

Copy link
Contributor

Choose a reason for hiding this comment

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

If you go to https://github.com/matrix-org/matrix-react-sdk/pull/8449/files, you can see there still are changes; it seems to be the case that the original code used - while you used ... I would suggest resetting to develop (something like git reset --hard upstream/develop), changing the .tsx file, running yarn i18n and then seeing what happens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make sure I am not doing anything wrong, I created a new branch did the change in .tsx file. Then ran yarn i18n but nothing is changed in en_EN.json

@SimonBrandner
Copy link
Contributor

Closing in favour of #8455

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The em dash should be an actual em dash, not a dash
3 participants