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 Hebrew translation and BiDi support issues #101

Merged
merged 6 commits into from
Aug 9, 2020

Conversation

danielrozenberg
Copy link
Contributor

@danielrozenberg danielrozenberg commented Jul 26, 2020

Fixes #63
Fixes #64
Fixes #70

@danielrozenberg danielrozenberg changed the title Fix Hebrew translation placeholders Fix Hebrew translation issues Jul 26, 2020
@danielrozenberg danielrozenberg changed the title Fix Hebrew translation issues Fix Hebrew translation and BiDi support issues Jul 27, 2020
@rugk
Copy link
Owner

rugk commented Jul 28, 2020

Hi @danielrozenberg,
thanks for your first contribution to this project! 🎉 👍 🏅
I hope you'll like this project and enjoy hacking on it… 😃

Thanks a lot also for targeting multiple related issues at once. 😊

I'll have a look at this soon.

Note that the fix for #63 uses CSS attributes that are currently only supported in Firefox (inset-inline-start and inset-inline-end, which are BiDi-supportive alternatives to using left and right respectively), which is why I didn't send a separate pull request to emoji-mart

Yeah, okay, that seems to be unfortunate then, but it's good for now as this add-on is only available in Firefox yet (see #54).
Remember you could also use predefined strings such as @@bidi_dir, as the examples show also for padding or so. Though I'm not sure whether that works in other browser's (= Chrome/iums) WebExtension implementation…


Requesting a potential review/test from @shanirub (if you want). Unless there are objections, this will be merged soon.

To test out the code, you need to clone the repo locally (with git submodules) and load it into Firefox. Fortunately, that is quite easy without having to install additional tools. In the contributing guide, you can see how to do that.


BTW if you care to be notified about translation updates subscribe to #27. 😃

Also notice there are more things to translate like the add-on listings on AMO (addons.mozilla.org) etc. See the contributing guide for more information.

But as your PR is mostly a technical fix, I'll treat it as such going forward… 😃

@danielrozenberg
Copy link
Contributor Author

Remember you could also use predefined strings such as @@bidi_dir, as the examples show also for padding or so. Though I'm not sure whether that works in other browser's (= Chrome/iums) WebExtension implementation…

Looks like @@bidi_start_edge is supported in Chrome as well, unfortunately I won't be able to make this change and test it out before the weekend. Of course, you can merge my PR since the extension is Firefox only for now, and have it changed when it becomes relevant (or not changed if Chrome decides to support these attributes 😝)

thanks for your first contribution to this project! 🎉 👍 🏅

my pleasure 😊

@rugk
Copy link
Owner

rugk commented Jul 28, 2020

All right, we are not in a hurry (no release is planned or so in the next time), so I'll leave this PR open for now…

@danielrozenberg
Copy link
Contributor Author

Updated to use the @@bidi messages:

LTR RTL
Screen Shot 2020-08-01 at 15 06 53 צילום מסך 2020-08-01 בשעה 14 59 43
Screen Shot 2020-08-01 at 15 07 07 צילום מסך 2020-08-01 בשעה 15 03 20
Screen Shot 2020-08-01 at 15 07 17 צילום מסך 2020-08-01 בשעה 15 03 29

src/common/common.css Outdated Show resolved Hide resolved
Copy link
Owner

@rugk rugk left a comment

Choose a reason for hiding this comment

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

Code review LGTM.

@danielrozenberg
Copy link
Contributor Author

Merge this whenever you feel like it 🙂

@rugk
Copy link
Owner

rugk commented Aug 9, 2020

Okay, tested and looks fine to me…

grafik
grafik

(Except of the screenshot for the emoji search, but that currently is not localized anyway.)

@rugk rugk merged commit 1d1c5c7 into rugk:master Aug 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants