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

Farsi translation & RTL support #5961

Closed
wants to merge 4 commits into from
Closed

Farsi translation & RTL support #5961

wants to merge 4 commits into from

Conversation

SidMorad
Copy link
Contributor

@SidMorad SidMorad commented Jun 20, 2017

According to Wikipedia(https://en.wikipedia.org/wiki/Persian_language)
There are approximately 110 million Farsi speakers worldwide, with the language
holding official status in Iran, Afghanistan, and Tajikistan.

Note: Farsi is a Right to Left (RTL) language, therefore we need proper CSS for displaying text correctly in html pages.
and here is rtl.css example that needs to be included(dynamically) if current language is a RTL language:

html {
  direction: rtl;
}

.col-xs-1, .col-xs-2, .col-xs-3, .col-xs-4, .col-xs-5, .col-xs-6, .col-xs-7, .col-xs-8, .col-xs-9, .col-xs-10, .col-xs-11, .col-xs-12 {
  float: right;
}

.modal-footer {
  text-align: left;
}

.dropdown-menu {
  text-align: right;
}

th {
  text-align: right;
}

@media (min-width: 768px) {
  .dl-horizontal dt {
    float:right;
  }
}

and here is a condition example in index.html
<link rel="stylesheet" ng-href="{{langKey === 'fa' ? 'content/css/rtl.css?v=2' : ''}}">

This commit assumes you will take care of HTML/RTL issues yourself, and
will show translations in correct format when text direction is set to rtl.

Please let me know if something is missing? or any improvement needed?

Thanks!

  • Please make sure the below checklist is followed for Pull Requests.

  • Travis tests are green

  • Tests are added where necessary

  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

According to Wikipedia(https://en.wikipedia.org/wiki/Persian_language)
There are approximately 110 million Farsi speakers worldwide, with the language
holding official status in Iran, Afghanistan, and Tajikistan.

Note: Farsi is a Right to Left (RTL) language and needs proper CSS consideration:
e.g. <html dir="rtl">
e.g.2 <div style="float: right;">
e.g.3 <div style="text-align: right;">

This commit assumes you will take care of HTML/RTL issues yourself, and
will show translations in correct format when direction is set to rtl.

Cheers!
@deepu105
Copy link
Member

Thanks for the translation @SidMorad
This is our first RTL language and hence I think we need make sure it works well like other languages. Let me take a look and think about a solution

@jdubois
Copy link
Member

jdubois commented Jun 20, 2017

Awesome !!!! That's really an important feature.

@SidMorad
Copy link
Contributor Author

I would like to point out, Ionic-team done really good job recently(as of version 3.4.2) regards to RTL support. It seems their solution is heavily depends to using SASS, but you may find some useful information at ionic-team/ionic/issues/11211.

@deepu105 I look forward to see your solution, in time between, I think adding RTL support and adding Frasi translation better to be two independent pull requests, don't you think? plus suggested solution must work with ease for next RTL language too, which means no dependency between specific language and RTL support.

@jdubois Thanks for saying supporting RTL is an important feature. As a RTL developer, I get used to the fact that supporting RTL is not a high priority task! ؛-)

Thanks for creating JHipster! JHipsters!

@deepu105
Copy link
Member

@SidMorad since you already have a solution and it looks good could you make a PR with it. We need to support it in botg ng1 and ng2± first before we support RTL languages

@SidMorad
Copy link
Contributor Author

@deepu105 My solution is pretty simple and is kind of hard-coded, let's look at this line:

<link rel="stylesheet" ng-href="{{ langKey === 'fa' ? 'content/css/rtl.css?v=2' : '' }}">

And for supporting next RTL language, let's say is Hebrew language, then we need to change it to:

<link rel="stylesheet" ng-href="{{ langKey === 'fa' || langKey === 'he' ? 'content/css/rtl.css?v=2' : '' }}">

Plus If a RTL language is not between selected languages, then (I assume) majority of developers don't expect/want to see such a line in their index.html or a rtl.css file in their content/css directory. am I wrong?

So I suggest someone who is more familiar with code base, work on this, and provides better/clever way to support RTL when is necessary.

@deepu105
Copy link
Member

@SidMorad that's easy to solve in the generator base we need to add another attribute rtl: true to the language object with RTL and then use that in the language constant in the generated code as well that way it's easier to implement this and we need to add the line only when there is at least 1 RTL language selected. You give it a shot if you are struck ill help you

@SidMorad
Copy link
Contributor Author

@deepu105 thanks for your support, sure I will give a shot this weekend. but no promise for contribute in next week, because we are suppose to release version 1 of our product by end of next week.

Note: In Iran Thursday and Fridays are weekend days! plus we use Persian calendar, another source of troubles for us as local developers! :-) sometimes I think of working in another country and tada! all problems solved!: no deal with RTL support, different calendar, unfriendly government!, etc! :-)

Note: After adding next RTL language, following methods need to include new added language.
 - generators/generator-base.js#isRTLSupportNecessary
and
 - generators/client/templates/angularjs/src/main/webapp/app/blocks/handlers/_translation.handler.js#isRTL

Cheers! (Note: Beer is forbidden here! can you imagine that!!
               I have to bring up a glass of tea instead! :-))
@SidMorad
Copy link
Contributor Author

@deepu105 Regards to RTL support, I went to slightly different path that we talk about initially. please have a look and let me know what do you think? thanks.

Note: I did test 4 different scenarios:

  • Monolithic app + ng1 + en, fa
  • Monolithic app + ng1 + en, de
  • Monolithic app + ng1 + sass + en, fa
  • Monolithic app + ng1 + sass + en, de

@SidMorad
Copy link
Contributor Author

JHipster + Farsi Language + RTL support screenshot:

screenshot from 2017-06-22 16-43-48

@deepu105
Copy link
Member

deepu105 commented Jun 23, 2017 via email

Copy link
Member

@deepu105 deepu105 left a comment

Choose a reason for hiding this comment

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

The solution looks good. I have 2 comments. can you also add the same for ng2 code, inside the angular folder

}

// Returns true if passed language key is a Right-to-Left language key
function isRTL(langKey) {
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 get this from the existing language filter. See my comment below

@@ -36,6 +36,7 @@
'en': 'English',
'es': 'Español',
'et': 'Eesti',
'fa': 'فارسی',
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to change the pipe to below and then use it in the above service.

(function() {
     'use strict';
 
     angular
         .module('<%=angularAppName%>')
         .filter('findLanguageFromKey', findLanguageFromKey);
         .filter('findLanguageRtlFromKey', findLanguageRtlFromKey);
     var languages = {
        'ca': { name: 'Català' },
        ....
        'fa': { name: 'فارسی', rtl: true },
        ...
        'zh-tw': { name: '繁體中文' }
     }
     function findLanguageFromKey() {
         return findLanguageFromKeyFilter;
 
         function findLanguageFromKeyFilter(lang) {
             return languages[lang].name;
         }
     }
     function findLanguageRtlFromKey() {
         return findLanguageRtlFromKeyFilter;
 
         function findLanguageRtlFromKeyFilter(lang) {
             return languages[lang].rtl;
         }
     }
 })();

@@ -437,6 +452,7 @@ module.exports = class extends Generator {
{ name: 'Dutch', value: 'nl' },
{ name: 'English', value: 'en' },
{ name: 'Estonian', value: 'et' },
{ name: 'Farsi', value: 'fa' },
Copy link
Member

Choose a reason for hiding this comment

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

add a new attribute { name: 'Farsi', value: 'fa', rtl: true }, and use that in isRTLSupportNecessary instead of hardcoding the values

No need for hard coding RTL language key as of this revision.

Thanks to @deepu105 for suggesting the solution.
@SidMorad
Copy link
Contributor Author

@deepu105 Thanks for your solutions, I did apply your requested changes for AngularJs(1.x) module. Please let me know if something is missing?

For Angular(4.x) module I did nothing, and as I said earlier I am kind of busy in this week, so I can be just a minor helper if someone else want to continue to work on this, thanks.

@deepu105
Copy link
Member

deepu105 commented Jun 24, 2017 via email

@SidMorad
Copy link
Contributor Author

SidMorad commented Jun 24, 2017

@deepu105 I notice my mind is with this issue, while I am working on other issues, so I did add some progress for angular(4.x) module(which they did pass initial tests). should I push them too?

@deepu105
Copy link
Member

If you have done yes please

@SidMorad
Copy link
Contributor Author

I did test following scenarios as of revision 08dfb9a:

  • Monolithic app + ng2 + en, fa
  • Monolithic app + ng2 + en, de
  • Monolithic app + ng2 + no i18n

Please let me know if something is missing? or can be done better? thanks.

@deepu105
Copy link
Member

deepu105 commented Jun 24, 2017 via email

@deepu105 deepu105 changed the title Farsi translation Farsi translation & RTL support Jun 25, 2017
@deepu105
Copy link
Member

merged manually

@deepu105 deepu105 closed this Jun 25, 2017
@SidMorad SidMorad deleted the farsi-translation branch June 25, 2017 12:03
@jdubois jdubois modified the milestone: 4.6.0 Jul 6, 2017
@SidMorad
Copy link
Contributor Author

SidMorad commented Jul 10, 2017

I start using JHipster v4.6.0 and it's RTL support in one of my projects, and I notice we can have some improvements regards to RTL support. and here I'am going to document it for future developers who may need this.

First I would like to point out, according to this comment, bootstrap project may add RTL support officially as of version 4.2.0 or later. So if bootstrap 4.2.0 is out and has RTL support, then please ignore this comment, and follow bootstrap guide in this regards.

Bootstrap 4.0.0-alpha.6 is used by JHipster v4.6.0 and our primary technique for adding RTL support was watching <html dir="rtl"> attribute and applying necessary CSS depends to it's value. for example:

html[dir=rtl] th {
    text-align: right;
}

bright/bootstrap-rtl project is using the same technique(with supporting almost all differences between LTR & RTL). and as I am writing this, it is compatible with bootstrap 3.x version. so I think if you are looking for a complete RTL support, then following file can be your guide:
https://github.com/bright/bootstrap-rtl/blob/master/dist/css/bootstrap-rtl-ondemand.css

and in my opinion upgrading above file to bootstrap 4.x is not that much complex, you only need to search for differences and replace them with updated syntax. for example search for

.col-md-offset-

and replace it with

.offset-md-

Have fun RTLing! ؛-)

p.s. If you find a better way for adding RTL support? please let us know, thanks.

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

Successfully merging this pull request may close these issues.

3 participants