Add missing support for RTL languages

tombigel - November 23, 2008 - 07:29
Project:Conditional Stylesheets
Version:6.x-1.1
Component:Code
Category:bug report
Priority:normal
Assigned:Unassigned
Status:needs review
Description

I maintain a theme called Tendu, and we are just now working on a function that does something similar.
Your implementation is far better then our current approach.

My question - Can this module / technique be implemented only with a function in template.php (+ the lines in the info file)?
I would love to add it as a part to my theme, but I don't want the theme to include a modules/ directory by default.

btw - how does your module handles sub-themes? any special behavior, or does it work just like regular added css file?

Thanks
Tom

#1

JohnAlbin - January 6, 2009 - 09:02
Status:active» fixed

With sub-themes, the same rules apply to conditional stylesheets as to regular stylesheets. Sub-themes inherit the conditional stylesheets of their parent themes; and sub-themes can override or remove conditional stylesheets in the normal way.

And, yes, you can add this technique using a tendu_preprocess_page() and a tendu_theme() function. But its tricky. Zen 6.x-1.0-beta3 (and later) does this.

#2

tombigel - January 13, 2009 - 09:42

Thanks, I copied the relevant code from Zen and it works :)

One more question I forgot to ask:
In Drupal 6, when an RTL language is selected, for every stylesheet loaded Drupal looks for an additional stylesheet-rtl.css and adds it for incremental RTL changes right after it loads the regular stylesheet.
This rule does not apply to the stylesheets added by this module, I'll try to add it manually and post it here, but it should be a part of the module.

#3

JohnAlbin - January 13, 2009 - 15:57
Status:fixed» active

Um… not sure. Can't check right this sec, so I'll re-open this issue so I don't forget to check later.

#4

tombigel - January 13, 2009 - 20:03
Status:active» needs work

Hi again, I created a patch for automatically adding RTL CSS files when needed.
It was tested only on my local Tendu installation, I hope it works for you too.

I hope the patch is in the right format, never created one for a Drupal project myself.

AttachmentSize
conditional_styles_rtl.patch 1.37 KB

#5

JohnAlbin - January 19, 2009 - 12:21
Title:Is there a way to use this technique with template.php only?» Add support for RTL languages
Category:support request» feature request
Status:needs work» needs review

Thanks for the patch! I'll take a look at it soon.

#6

tombigel - January 19, 2009 - 13:42

btw - The code with the patch is already integrated in Tendu version 2.x-dev and 2.x beta1

#7

tombigel - January 22, 2009 - 08:34
Status:needs review» needs work

Found a bug in my code:
If "locale" module is not enabled, the constants LANGUAGE_RTL / LANGUAGE_LTR are not defined.
I tried to replace this line in my patch:

if ($language->direction == LANGUAGE_RTL){

With this line:

if (defined(LANGUAGE_RTL) && $language->direction == LANGUAGE_RTL){

But it doesn't work.

I need help with this one...

#8

tombigel - January 22, 2009 - 08:38

Sorry my bad - it was supposed to be:
defined('LANGUAGE_RTL')

So this line:

if (defined('LANGUAGE_RTL') && $language->direction == LANGUAGE_RTL){

Works!

#9

tombigel - January 22, 2009 - 08:46

You can't add a file when editing a comment here, so here's the updated patch...

AttachmentSize
conditional_styles_rtl.patch 1.4 KB

#10

kiamlaluno - January 22, 2009 - 08:59

Rather than using all those backslashes to escape the string delimiter inside it, I would use a different string delimiter.

AttachmentSize
conditional_styles_rtl_094534.patch 1.4 KB

#11

kiamlaluno - January 22, 2009 - 09:11

I changed a couple of string delimiters which should not have been changed.

AttachmentSize
conditional_styles_rtl_094535.patch 1.4 KB

#12

tombigel - January 22, 2009 - 09:10

ok.
I just copied their original "output .=" line...

#13

JohnAlbin - May 4, 2009 - 11:13
Status:needs work» needs review

So we have a choice of lots of \" or lots of ' . ' in that line. Neither are ideal, but I still find the \" easier to read.

Thanks for the patch, Tom! Can I get someone to confirm the patch in #9 actually works? :-)

#14

JohnAlbin - May 25, 2009 - 12:06
Category:feature request» bug report

This is the same as Tom's patch in #9 but conforms to Drupal's coding standards (with regards to spaces). Also, given how drupal_add_css() works, I've removed the check for defined('LANGUAGE_RTL') as it seems unnecessary.

AttachmentSize
rtl-337975-13.patch 1.45 KB

#15

JohnAlbin - May 25, 2009 - 12:43
Title:Add support for RTL languages» Add missing support for RTL languages

Tom, I just re-read your comment in #7 above, LANGUAGE_RTL and LANGUAGE_LTR are not defined in locale.module, they are defined during the bootstrap. http://api.drupal.org/api/constant/LANGUAGE_RTL/6

So, I don't understand what errors you were encountering.

#16

JohnAlbin - May 25, 2009 - 14:57
Status:needs review» fixed

Ok, I created a fake language for en-us that is RTL. Which, let me tell you, gives very interesting results. :-)

But its enough for me to test this patch. Everything seems to be working.

Thanks for the patch, Tom! http://drupal.org/cvs?commit=216204

#17

tombigel - May 28, 2009 - 17:09

@JohnAlbin: Maybe I was wrong about what caused the symptom, I just know what fixed it.
And anyway, checking whether a constant is defined before using it is better practice.

#18

System Message - June 11, 2009 - 17:10
Status:fixed» closed

Automatically closed -- issue fixed for 2 weeks with no activity.

#19

aliadnan - September 18, 2009 - 10:56
Version:6.x-1.0» 6.x-1.1
Status:closed» needs review

@JohnAlbin Thank you for this useful module.

I'm sorry to reopen this issue but I experiencing problems with my rtl site although I'm using 6.x-1.1 and Tom recent rtl patch if (defined('LANGUAGE_RTL') && $language->direction == LANGUAGE_RTL) should work.

my case:
- site has 2 languages enabled, one ltr and one rtl.
- the site default is the ltr one.
- rtl language negotiation set to path prefix only
- the module should load one of the files, ie6.css or ie7.css if on IE and if language is rtl then load it with it -rtl.css version.

the issue:
If the browser is IE (whether 6 or 7) then the conditional style always load both files (ie6.css and ie6-rtl.css) whether page language is rtl or ltr.

I tried to make the language condition at line 78 to be $language->direction == LANGUAGE_RTL but didn't work.

I'm out of reasons why this module is working for me.

#20

aliadnan - September 18, 2009 - 16:37

Update

The issue is clear now

The browser keeps conditional style load both files (ie6.css and ie6-rtl.css) whether page language is rtl or ltr until you "clear cache" from admin>settings>performance.

Once cache cleared you will have no -rtl.css at the ltr . BUT again it gets sticky once you switch to rtl and then it gets back to the first state with no difference (-rtl.css) loads whether on rtl or ltr page until you clear system cache.

drupal core similar functionality adds a dummy query string to keep forcing loads of new copy of files with the url change. http://api.drupal.org/api/function/drupal_get_css/6

$query_string = '?'. substr(variable_get('css_js_query_string', '0'), 0, 1);

#21

alexanderpas - September 20, 2009 - 14:28

RTL cache overwrote LTR cache (or vice-versa)

this patch should fix it.

AttachmentSize
conditional_styles-rtl_caching-337975-21.patch 1.09 KB

#22

tombigel - September 29, 2009 - 01:47

@aliadnan:

Weird, I haven't seen this behavior in any of the sites I built on Tendu, and never had a bug report about it.

On the other hand I never used "Path Prefix" in any of my sites. Does Drupal cache things differently in this case?
Or maybe you are doing something else like "Aggressive Mode" caching?

In any case, I'll mark the patch posted here as yet another thing I need to add to Tendu when I have the time... thanks.

 
 

Drupal is a registered trademark of Dries Buytaert.