[i18n] RTL support #690

sepans · 2020-04-14T06:09:51Z

Fixes: Support for Right-to-left languages (Persian, Arabic, Hebrew, Urdu etc.)

Changes (in 4 commits so review by commit is easier):

Adds rtl support: processing/p5.js-website@b65a373 (Mirrors the page for right to left languages)
- Adds rtl parameter to Gruntfile
- Based on rtl adds dir='rtl' to <html> tag
- Adds .rtl class to <html> tag (Not ideal but better than restructuring the entire page)
Slightly restructures homepage 'Hello' and search bar to avoid float: right as it breaks rtl: processing/p5.js-website@7360072
Some css overrides: processing/p5.js-website@23d0c46
Moved sidebar to layout processing/p5.js-website@6da4690
The sidebar on the homepage was inside a flex container and it was working fine but on the other pages it was not. I had to add flex to the top container on every page but instead of that took the opportunity and moved the {{> sidebar }} to the layout and added flex to the container in the layout to make the templates dryer.
The diff looks large but the change was mechanical running a regex like:

<div id="([a-z\-]+)">[\n\s]+\{\{> sidebar\}\}[\n\s]+<div class="column-span">

Screenshots

more screenshots

Mobile

Note:
I am also working on Persian translation but decided to do 2 separate PRs to make the review easier for you. So this PR does not include fa.yml etc and the screenshots above with Persian text in them are just for demonstration. This PR should not cause any change to the website.

Testing: I tried to check all the pages on different screen sizes and browsers but I might have missed a page since I don't know my way around the website very well.

Doesn't work with `dir='rtl'` source: https://www.w3.org/International/geo/html-tech/tech-bidi.html#ri20030728.093644822

…iner

sepans · 2020-04-14T06:26:52Z

src/assets/css/main.css

@@ -1168,7 +1168,6 @@ img.gallery-img {
 #showcase-page .showcase-intro h1 {
  font: italic 900 14.5vw "Montserrat", sans-serif;
  color: #ED225D;
-  text-align: left;


👆 was not doing anything except breaking rtl 😄

limzykenneth · 2020-04-15T12:46:17Z

I don't think there's currently plans to officially support additional languages other than the ones that are already live or are being worked on (ie. Korean and Hindi).

Although this PR do add much of the things I would like to see on the site, we do have the policy of usually requiring an issue to be opened before a PR should be filed so that discussions about any particular features are hammered out before any work is done. I'm inclined to review this PR as is now but next time please do open an issue first. Thanks.

sepans · 2020-04-16T01:47:32Z

Thanks for your comment @limzykenneth 🙌 If there is no plan for adding additional languages it is fine to close this PR, or just cherry-pick the needed changes.

Maybe a more urgently needed change is to update the i18n doc with this policy before someone else starts translation for another language.

limzykenneth · 2020-04-16T16:21:18Z

@sepans Translation of the website is always welcomed, the only difference is whether a language is officially supported or not. An officially supported language will be listed on p5js.org while an unoffical one can be hosted by the translator wherever they wish. The i18n documentation aims to make the technical aspect of the translation clear and any suggestion of new official translations should follow the PR workflow documented here.

limzykenneth

Some questions inline

limzykenneth · 2020-04-16T16:22:24Z

Gruntfile.js

@@ -81,6 +81,10 @@ module.exports = function(grunt) {
    assemble: {
      pages: {
        options: {
+          rtl: function() {
+            var rtlLangs = ['ar', 'fa', 'he', 'ur'];


Instead of listing all these languages here, can this just be a blank array and whenever a rtl language is added the translator can add the appropriate language code themselves?

Yes totally. I feel var rtlLangs = ['ar', 'fa', 'he', 'ur'] is more self-documenting whereas var rtlLangs = [] is a bit of mystery why we lookup inside an empty list (assuming this doesn't affect performance since it is just a build time script)
And again we need to document this somewhere etc, but your call!

We can expand on the contributor docs to include a section about RTL languages. Some line comments can be added here as well if you feel that just leaving it as an empty array is potentially confusing. Performance won't be a concern in any case here.

limzykenneth · 2020-04-16T16:23:20Z

src/assets/css/main.css

+
+/* ======= Right-to-left modifications ======== */
+
+html.rtl #lockup {


Can the selector be selecting the lang attribute on the html tag instead of using a class on the tag?

Yes that is possible. The downside is that for each new rtl language a lang item needs to be added (and possibly someone spending some time to figure it out and/or documenting it). To me, rtl class seems to be a small reasonable abstraction to cover them all.

I would like to try out just using the lang attribute for now, perhaps somewhere down the line we can look into automating the build to handle some of the details that needs to be added manually for now.

Well, what language should I put there? I can arbitrarily add either lang(fa) or lang(he), or all of them but in the above comment you suggested removing them all from the Gruntfile.js

lang(fa) for now would be fine, do add a line comment about what the function of it is just above.

Sorry about the delay. I removed elements from rtlLangs array as you suggested, but as for adding :lang(fa) I think it is exactly opposite of what I did for rtlLangs: adding code specific to a language that is not supported yet.
And also it gets ugly as we eventually add multiple rtl languages and chain css for all the language.

sepans · 2020-04-16T18:23:48Z

Thanks for the explanation. I still argue for adding that 'official languages' to the i18n document because I personally could have translated a lot of more examples instead of translating the homepage if I knew that. Maybe an RFC if a better place to have this discussion.

limzykenneth · 2020-04-18T14:25:50Z

@sepans Would you like to submit a modification to the translation docs? Only thing to keep in mind is that while we may not officially support some languages at the moment, any translation effort is always welcomed.

sepans added 4 commits April 14, 2020 00:38

[rtl] Add dir=rtl for right to left languages

b65a373

[rtl] Use flex instead of float for 'Hello' and search input

7360072

Doesn't work with `dir='rtl'` source: https://www.w3.org/International/geo/html-tech/tech-bidi.html#ri20030728.093644822

[rtl] rtl css style overrides

23d0c46

[rtl] Move sidebar to layout for all pages and use flex for conta…

6da4690

…iner

sepans assigned lmccart Apr 14, 2020

sepans requested a review from limzykenneth April 14, 2020 06:17

sepans commented Apr 14, 2020

View reviewed changes

limzykenneth unassigned lmccart Apr 15, 2020

limzykenneth reviewed Apr 16, 2020

View reviewed changes

[rtl] PR comments

b5fd450

lmccart changed the base branch from master to main June 14, 2020 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[i18n] RTL support #690

[i18n] RTL support #690

sepans commented Apr 14, 2020 •

edited

Loading

sepans Apr 14, 2020

limzykenneth commented Apr 15, 2020

sepans commented Apr 16, 2020

limzykenneth commented Apr 16, 2020

limzykenneth left a comment

limzykenneth Apr 16, 2020

sepans Apr 16, 2020 •

edited

Loading

limzykenneth Apr 18, 2020

limzykenneth Apr 16, 2020

sepans Apr 16, 2020

limzykenneth Apr 18, 2020

sepans Apr 19, 2020

limzykenneth Apr 19, 2020

sepans May 17, 2020

sepans commented Apr 16, 2020

limzykenneth commented Apr 18, 2020


		/* ======= Right-to-left modifications ======== */

		html.rtl #lockup {

[i18n] RTL support #690

Are you sure you want to change the base?

[i18n] RTL support #690

Conversation

sepans commented Apr 14, 2020 • edited Loading

Screenshots

Mobile

Choose a reason for hiding this comment

limzykenneth commented Apr 15, 2020

sepans commented Apr 16, 2020

limzykenneth commented Apr 16, 2020

limzykenneth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sepans Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sepans commented Apr 16, 2020

limzykenneth commented Apr 18, 2020

sepans commented Apr 14, 2020 •

edited

Loading

sepans Apr 16, 2020 •

edited

Loading