globaleaks / globaleaks-whistleblowing-software

GlobaLeaks is free, open-source whistleblowing software enabling anyone to easily set up and maintain a secure reporting platform.
https://www.globaleaks.org
Other
1.23k stars 269 forks source link

Adopt rtlcss in order to deal with automatic handling of rtl CSS files whenever possible #1621

Closed evilaliv3 closed 8 years ago

evilaliv3 commented 8 years ago

The rtlcss by @MohammadYounes seems to work really well in handling automagically RTL conversion of CSS written for LTR.

That library is already used by wordpress and is under evaluation by the bootstrap: https://github.com/twbs/bootstrap/issues/19555

It would be interesting to adopt it in the project instead of caring of all manually

NSkelsey commented 8 years ago

Are you thinking that this will be implemented as a compiled CSS file that is used only when an RTL language is chosen?

This seems like a win-win.

evilaliv3 commented 8 years ago

In general i do not like that much the solutions that dupliate the file cause they leak information, like that the user is an RTL user, leak a lot, but we can evenually serve anyhow both the files and use one of them on the client. The same apply sadly for the translations that generally are really bigger, but we can evaluate to gzip them inside the client so to pack them and reduce their size.

evilaliv3 commented 8 years ago

Having a single file is an option discussed by in https://github.com/MohammadYounes/rtlcss/issues/34 and doable with https://github.com/rtsao/postcss-rtlcss-combined but as rightly identified by @MohammadYounes that could lie various isues due to the css hierarchies so for the meantime let's simply accept to have two different files.

NSkelsey commented 8 years ago

@evilaliv3 you are in general referring to the issue of analysis of HTTPS traffic. That issue is larger in scope than one or two CSS files.

My understanding of the Tor browser was that this issue is addressed by noise generated in the Tor network.

MohammadYounes commented 8 years ago

There is also postcss-inline-rtl which inlines the minimum amount of RTL CSS you need and solves the specifity problem.

evilaliv3 commented 8 years ago

thanks @MohammadYounes! we will keep that in mind!

evilaliv3 commented 8 years ago

Implemented! if you would like to cite us as your early users we would love that! :)

GlobaLeaks, the first open-source whistleblowing framework built using AngularJS, Bootstrap and Twisted!

evilaliv3 commented 8 years ago

screenshot from 2016-04-02 01 28 06

MohammadYounes commented 8 years ago

Sure, update the readme and send a PR.

Also RTL-bootstrap might be of interest to you (with JS components RTL support)

evilaliv3 commented 8 years ago

Thanks.

Yes all our interest in rtlcss is mainly for boostrap, but we found RTL-bootstrap still not not handled that well and not aligned with bootstrap 3.3.6 and the latest patches; Would it take time to do a proper clean patching?

p.s. i've already tried also postcss-inline-rtl but it seems to have some defect:

screenshot from 2016-04-02 02 25 39

The code of the nav is a standard use of some Bootstrap classes so probably simply using rtlcss works best now:

<ul class="nav nav-pills nav-stacked">
  <li data-ng-class="active.content">
    <a href="#/admin/content">
      <i class="glyphicon glyphicon-chevron-right"></i>
      <span data-translate>General settings</span>
    </a>
  </li>

  <li data-ng-class="active.users">
    <a href="#/admin/users">
      <i class="glyphicon glyphicon-chevron-right"></i>
      <span data-translate>User management</span>
    </a>
  </li>
...
</ul>

\cc @jakob101

jakob101 commented 8 years ago

I'd like to see what's the issue with postcss-inline-rtl, and resolve it :) I'm out of the country but will be back on monday. Can we talk more then?

evilaliv3 commented 8 years ago

sure @jakob101, reach me even on skype if you like; my nick handle is 'evilaliv3'

MohammadYounes commented 8 years ago

I'll rebase with bootstrap master later today.

evilaliv3 commented 8 years ago

great @MohammadYounes!

let me know if i can help somehow. what would be great instead of a simple rebase would be to do a patching with few organized commits atop of a clean 3.3.6 that is likely to be the final stable version of bootstrap;

probably something done with rtlcss/postcss-inline-rtl is likely to be integrated in bootrap [1] and would be a great benefit for the community out there: https://github.com/morteza/bootstrap-rtl/issues/98

[1] https://github.com/twbs/bootstrap/issues/19555

MohammadYounes commented 8 years ago

Currently it's 3.3.5 (comparison). It has 1 extra feature that wasn't merged.

I thought about sending a PR, but they made it clear no RTL support will be added to v3

evilaliv3 commented 8 years ago

Okay, my suggestion is to squash it a little having in this order to have atop of 3.3.6

i'm unsure if it would be a good result to relase it using postcss-inline-rtl so to have a single file that can do both depending on the rtl direction.

MohammadYounes commented 8 years ago

Yeah, using postcss-inline-rtl will make it easier for distribution. I'll do it later tonight :)

MohammadYounes commented 8 years ago

I pushed inline-rtl branch, to get a working local build, postcss-inline-rtl requires the following updates:

:sleeping: :wave:

MohammadYounes commented 8 years ago

A preview of the docs can be found @ http://mohammadyounes.github.io/RTL-bootstrap/inline

Note: I added a switch direction link to the side menu capture

jakob101 commented 8 years ago

You, my man, are great!

Thanks, I missed that.

Hope you had a good night sleep :)

On 3 April 2016 at 01:50, Mohammad Younes notifications@github.com wrote:

I pushed inline-rtl https://github.com/MohammadYounes/RTL-bootstrap/tree/inline-rtl branch, to get a working local build, postcss-inline-rtl requires the following updates:

[image: :sleeping:] [image: :wave:] .

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/globaleaks/GlobaLeaks/issues/1621#issuecomment-204828030

MohammadYounes commented 8 years ago

Thanks :)

evilaliv3 commented 8 years ago

This is amazing!

other suggestions that comes to my mind:

question: we are using https://github.com/angular-ui/bootstrap; is there some particular tweak you did to the official bootstrap JS that should be probably be done there (p.s. i do not want you to look at the whole code, simply guess given your experience).

reference ticket: https://github.com/angular-ui/bootstrap/issues/4762

evilaliv3 commented 8 years ago

thanks @MohammadYounes you are doing really an amazing job! :)

MohammadYounes commented 8 years ago

@evilaliv3 Thanks :)

The only change to JS was in tooltip placement since it was done via JS not CSS.

For Angular UI, the same needs to be done here.

I'm still waiting for @jakob101 to update postcss-inline-rtl before users can actually build this!

MohammadYounes commented 8 years ago

I just noticed that carousel transition is not working as expected :(

I'll check it later tonight.

evilaliv3 commented 8 years ago

ah! anyway i'm thinking to change back from angular-ui/bootstrap to http://mohammadyounes.github.io/RTL-bootstrap/inline!

let's do it :) as soon that it will be ready and aligned with the 3.3.6 i will one shot do the change!

evilaliv3 commented 8 years ago

ah no damn, for various reasons we would still need to run us anguar-ui/bootstrap for the more control it offers us in angular :( i will so try to build a patch for the tooltips in RTL as you did in http://mohammadyounes.github.io/RTL-bootstrap/inline

jakob101 commented 8 years ago

@MohammadYounes I will update tomorrow, I'm still out of the country! :)

On Sun, Apr 3, 2016 at 5:51 AM -0700, "Giovanni Pellerano" notifications@github.com wrote:

ah no damn, for various reasons we would still need to run us anguar-ui/bootstrap for the more control it offers us in angular :( i will so try to build a patch for the tooltips in RTL as you did in http://mohammadyounes.github.io/RTL-bootstrap/inline

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

MohammadYounes commented 8 years ago

@jakob101 Take your time :)

@evilaliv3 I updated the branch:

Now it's fully converted :)

evilaliv3 commented 8 years ago

Great @MohammadYounes!

I was about to integrate it in globaleaks but what we miss is:

if it is good and you also think this would help easy the process what we can do is too fork your project under the GlobaLeaks organization; assign you permissions to commit on the specific repository where to collaborate; i can handle the publishing on the bower and sign the releases that could have a numbering 3.3.6-our.numbering.

this way then for our personal use i will do the same for angular-ui/bootstrap-rtl

let me know if this plan is ok for you.

evilaliv3 commented 8 years ago

what i was about to do is to use:

"bootstrap": "git@github.com:MohammadYounes/RTL-bootstrap.git#f10c0c2afa0ace9a009cc3eb8c9524539d466383"

but this forces me to download the whole repository and in addition the repository currently miss the build in the /dist directory (i would suggest that every time you bild it you commit the build in a separate new commit with the bumb the release.

i've a doubt in the release numbering as bootstrap uses X.Y.Z but semver does not allow X.Y.Z.K with a forth number like 1.2.3.4: https://github.com/mojombo/semver/issues/168 @MohammadYounes @jakob101 any suggestion?

evilaliv3 commented 8 years ago

probably the idea that comes to my mind is to always tag the same version of the current release of bootstrap, and whenever we apply changes simply delete the tag and re-tag it with the same current numbering. but i do not know if bower will be then able to do upgrade recognizing that the tag is different

MohammadYounes commented 8 years ago

@evilaliv3 I will commit the dist along with the updated version of postcss-inline-rtl (see #4).

I think this would be the last version in v3, and for v4 they mentioned it's to be added in a future minor release:

RTL support is currently slated for a future minor release after v4.0.0 has a stable release.

For now, I believe it's easier to have the dist manually included in your repo.

MohammadYounes commented 8 years ago

@evilaliv3 tagged under 3.3.6