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

Bootstrap right-to-left integration #178

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Bootstrap right-to-left integration #178

wants to merge 2 commits into from

Conversation

fisharebest
Copy link

This is a pull-request for #177 - RTL support for the bootstrap3 integration.

The only changes are to add new CSS rules with a [dir=rtl] prefix, so it shouldn't break any existing pages.

Bootstrap itself is heavily LTR-only, and it is assumed that some fixup for this is already present. (https://github.com/morteza/bootstrap-rtl is widely used for this purpose.)

I've split the PR into two commits (the second one just fixes whitespace) for easy reviewing.

I've tested this on the latest Firefox, Chrome, and IE8/9/10.

I have not tested the changes to DTTT_print_info, because I can't work out where it is used.
The DTFC_RightHeadWrapper/DTFC_LeftHeadWrapper rule resets 3 of the 4 corners. Again, I can't work out where this is used, but the changes should work.

@DataTables
Copy link
Collaborator

Excellent - thanks! Are you happy to have these changes released under the MIT license?

I'm going to be updating this file to be SCSS based in the next month or so which will make this kind of support much easier I think - better Bootstrap integration for all DataTables extensions is on my to do list - it is a mess atm!

@fisharebest
Copy link
Author

Are you happy to have these changes released under the MIT license?

Sure. If you need me to add my name to a CLA, etc., just point me at it.

@DataTables
Copy link
Collaborator

I'm happy with the above as acceptance. That's all the CLA would say anyway :-).

I'll try it out shortly and merge it in after that.

@DataTables
Copy link
Collaborator

Your RTL integration is rather more complete that mine from DataTables' core stylesheet! I've got it so that it simply handles RTL without actually breaking anything (for example all the layout components stay where they are), while you've gone for the complete flip.

I need to have a little think on what the correct approach here is so I can harmonise the two. I'm quite tempted to suggest that there should be an RTL stylesheet since the majority of users won't need these extra styles.

Would be interested in your own thoughts (are you a native RTL language speaker / reader?).

@fisharebest
Copy link
Author

while you've gone for the complete flip

RTL pages need to be exact mirrors of the LTR pages. Here is an example using dataTables with jQuery-UI:

https://dev.webtrees.net/demo-dev/indilist.php?alpha=%2C&ged=demo&lang=ar
https://dev.webtrees.net/demo-dev/indilist.php?alpha=%2C&ged=demo&lang=en-GB

The localisation of the pagination buttons is currently broken, but we're moving to Bootstrap so no point working on it further.

are you a native RTL language speaker / reader?

I'm not, but I've spent 10 years on multi-lingual projects, and work closely with native Hebrew and Arabic speakers. I think I've come across most RTL issues in that time...

The issues I've reported came directly from our Hebrew translator/tester.

I'm quite tempted to suggest that there should be an RTL stylesheet since the majority of users won't need these extra styles

My experience of using extra RTL stylesheets is that they are harder to keep up-to-date and maintain. If you have a single stylesheet where each "left/right" rule is immediately followed by its RTL cousin, things are much easier in the long term.

It's also easier for users/developers to have a single file to include. A module/library written using datatables plus the plugin won't need to know if it is being used in a project that has RTL.

IIRC, you were considering a move to SASS/LESS? If so, try to starting thinking in terms of "start" and "end", rather than "left" and "right".

Also, if you have a spare few hours, start reading twbs/bootstrap#13564 and follow the links... ;-)

@DataTables
Copy link
Collaborator

Great feedback - thanks. The downside to including RTL support in the main file is that it adds dead code weight for the majority of users. As a library author I feel obliged to keep the file size as small as possible - but that is a balance between file size and features.

IIRC, you were considering a move to SASS/LESS?

Certainly am. Current plan is to move the Bootstrap (Foundation, etc) integration out of this repo and into the individual extensions repos. More files, but with a download builder (coming soon) that shouldn't be an issue and will help improve how easy it is to create and test individual use cases. RTL could be a checkbox option for that, although I must admit I don't fancy doubling the number of CSS files...

@fisharebest
Copy link
Author

it adds dead code weight for the majority of users

You know your project's audience better than I do, but the counter-arguements are

  1. a billion (or so) people use RTL langauges, and we should be inclusive by default
  2. it's a micro-optimisation that trades complexity for an immeasurable gain

@DataTables
Copy link
Collaborator

Let me get back to you on this. I'll have a read though of some of the BS thread comments and links. It might be a little while before I can do so due to the amount of stuff going on, but this is something I would like to have supported. Just how exactly and what the best way of doing it is remains to be defined.

@elad
Copy link

elad commented May 4, 2015

@DataTables @fisharebest there's #51 which has been sitting in the pull requests for about a year and achieves the same thing plus FontAwesome and Glyphicons integration...

@fisharebest
Copy link
Author

FYI, I have updated/rebased this for the latest (1.10.7) code.

@elad - IMHO, your pull request does too many different/unrelated things. If a pull-request contains three changes, it is difficult for a project owner to merge one, reject one and discuss the other...

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