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

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

Closed
evilaliv3 opened this issue Apr 1, 2016 · 34 comments

Comments

@evilaliv3
Copy link
Member

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: twbs/bootstrap#19555

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

@evilaliv3 evilaliv3 added this to the 2016 April milestone Apr 1, 2016
@NSkelsey
Copy link
Contributor

NSkelsey commented Apr 1, 2016

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
Copy link
Member Author

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
Copy link
Member Author

Having a single file is an option discussed by in MohammadYounes/rtlcss#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
Copy link
Contributor

NSkelsey commented Apr 1, 2016

@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
Copy link

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

@evilaliv3
Copy link
Member Author

thanks @MohammadYounes! we will keep that in mind!

evilaliv3 added a commit that referenced this issue Apr 1, 2016
@evilaliv3
Copy link
Member Author

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
Copy link
Member Author

screenshot from 2016-04-02 01 28 06

@MohammadYounes
Copy link

Sure, update the readme and send a PR.

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

@evilaliv3
Copy link
Member Author

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
Copy link

jakob101 commented Apr 2, 2016

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
Copy link
Member Author

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

@MohammadYounes
Copy link

I'll rebase with bootstrap master later today.

@evilaliv3
Copy link
Member Author

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: morteza/bootstrap-rtl#98

[1] twbs/bootstrap#19555

@MohammadYounes
Copy link

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
Copy link
Member Author

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

  • a commit that rtlcss
  • few clean commit that have the eventual patchs
  • a last commit with the build

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
Copy link

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

@MohammadYounes
Copy link

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

  • adding float to the always convert list (PR) - without this justified button groups will be broken.
  • account for comment nodes when removing empty rules (PR) - without this you'll get a bunch of lint errors.
  • ability to extend the list of properties to convert (Issue) - without this directional icons are not flipped and carousel nav icons will be revered.

😴 👋

@MohammadYounes
Copy link

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
Copy link

jakob101 commented Apr 3, 2016

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: 😴] [image: 👋]
.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1621 (comment)

@MohammadYounes
Copy link

Thanks :)

@evilaliv3
Copy link
Member Author

This is amazing!

other suggestions that comes to my mind:

  • As bootstrap releases versions with numbering X.Y.Z, let's start releasing X.Y.Z-K or X.Y.Z.K depending if the first is standard or not; e.g. would you please release a 3.3.6-1
  • would you please tag a release, put in on bower and we will start using it!

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: angular-ui/bootstrap#4762

@evilaliv3
Copy link
Member Author

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

@MohammadYounes
Copy link

@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
Copy link

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

I'll check it later tonight.

@evilaliv3
Copy link
Member Author

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
Copy link
Member Author

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
Copy link

jakob101 commented Apr 3, 2016

@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
Copy link

@jakob101 Take your time :)

@evilaliv3 I updated the branch:

  • Fixed carousel by adding transition to the always convert list (PR).
  • Fixed tooltip positioning inside modal (commit).

Now it's fully converted :)

@evilaliv3
Copy link
Member Author

Great @MohammadYounes!

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

  • at least that it is published on bower; could you?
  • a tagged release would be lovely

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
Copy link
Member Author

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: semver/semver#168
@MohammadYounes @jakob101 any suggestion?

@evilaliv3
Copy link
Member Author

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
Copy link

@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
Copy link

@evilaliv3 tagged under 3.3.6

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

No branches or pull requests

4 participants