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.

Sign up for GitHub

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

Jump to bottom

[i18n] RTL support #690

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

[i18n] RTL support #690

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b65a373
[rtl] Add `dir=rtl` for right to left languages
sepans Apr 13, 2020
7360072
[rtl] Use `flex` instead of `float` for 'Hello' and search input
sepans Apr 13, 2020
23d0c46
[rtl] rtl css style overrides
sepans Apr 13, 2020
6da4690
[rtl] Move `sidebar` to layout for all pages and use `flex` for conta…
sepans Apr 13, 2020
b5fd450
[rtl] PR comments
sepans May 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ module.exports = function(grunt) {
assemble: {
pages: {
options: {
rtl: function() {
var rtlLangs = ['ar', 'fa', 'he', 'ur'];
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@sepans sepans Apr 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Member

Choose a reason for hiding this comment

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

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.

return rtlLangs.includes(this.language);
},
expand: true,
flatten: true,
helpers: ['<%= config.src %>/assets/js/translation.js'],
Expand Down
27 changes: 22 additions & 5 deletions src/assets/css/main.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,8 +619,9 @@ form {
color: #666;
}

#search {
float: right;
.hello {
display: flex;
justify-content: space-between;
}

#search .twitter-typeahead .tt-dropdown-menu {
Expand Down Expand Up @@ -1167,7 +1168,6 @@ img.gallery-img {
#showcase-page .showcase-intro h1 {
font: italic 900 14.5vw "Montserrat", sans-serif;
color: #ED225D;
text-align: left;
Copy link
Member Author

Choose a reason for hiding this comment

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

👆 was not doing anything except breaking rtl 😄

text-transform: uppercase;
}

Expand Down Expand Up @@ -2018,7 +2018,7 @@ footer {
#i18n-btn {
position: absolute;
top: 2.5em; /* temp promo, 4.0em */
right: 1em;
margin: 0 1em;
}
#i18n-btn a {
font-family: "Montserrat", sans-serif;
Expand Down Expand Up @@ -2166,7 +2166,6 @@ footer {
.column-span {
margin: 0 1em 0 1em;
padding: 0;
float: left;
}
#menu.top_menu,
#menu {
Expand Down Expand Up @@ -2471,3 +2470,21 @@ iframe {
display: inline;
}
}

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

html.rtl #lockup {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

left: auto;
right: 1.25em;
}

html.rtl #home-page #asterisk-design-element {
right: auto;
left: 20%;
}

@media screen and (max-width: 719px) {
html.rtl .sidebar-menu-icon {
float: left
}
}
2 changes: 1 addition & 1 deletion src/templates/layouts/default.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!DOCTYPE html>
<html class="no-js" lang="{{language}}">
<html class="no-js {{#if rtl}}rtl{{/if}}" lang="{{language}}" {{#if rtl}}dir="rtl"{{/if}}>
<head>
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
Expand Down
18 changes: 9 additions & 9 deletions src/templates/pages/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ slug: /

<div class="column-span">
<main id="home">

<form id="search" method="get" action="https://www.google.com/search">
<input type="hidden" name="as_sitesearch" value="p5js.org" >
<input id="search_button" type="submit" aria-label="Search" class='sr-only'>
<input tabindex="1" id='search_field' type="text" size="20" placeholder="Search p5js.org" name="q" >
<label class="sr-only" for="search_field">Search p5js.org</label>
</form>

<h1>{{#i18n "p1xh1"}}{{/i18n}}</h1>
<div class="hello">
<h1>{{#i18n "p1xh1"}}{{/i18n}}</h1>
<form id="search" method="get" action="https://www.google.com/search">
<input type="hidden" name="as_sitesearch" value="p5js.org">
<input id="search_button" type="submit" aria-label="Search" class='sr-only'>
<input tabindex="1" id='search_field' type="text" size="20" placeholder="Search p5js.org" name="q">
<label class="sr-only" for="search_field">Search p5js.org</label>
</form>
</div>
<p style="margin-top:1em">{{#i18n "p1x1"}}{{/i18n}}</p>
<p>{{#i18n "p1x2"}}{{/i18n}}</p>
<span class='button_box'><a href="https://editor.p5js.org">{{#i18n "start-creating"}}{{/i18n}}</a></span>
Expand Down