Make WordPress Core

Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#17603 closed enhancement (fixed)

Twenty Eleven theme options page (RTL)

Reported by: rasheed's profile rasheed Owned by: ryan's profile ryan
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.2
Component: RTL Keywords: has-patch
Focuses: Cc:

Description

Hello,

Please check the attached snapshot, some float right is needed at this page:

wp-admin/themes.php?page=theme_options

These changes make it look better:

.image-radio-option label {

display: block;
float: right;
margin: 0 2px 20px 30px;
position: relative;

}

Attachments (8)

Untitled-2.jpg (86.9 KB) - added by rasheed 13 years ago.
17603.diff (938 bytes) - added by ryan 13 years ago.
17603.2.diff (1.1 KB) - added by ryan 13 years ago.
body.rtl
wp32rc1.PNG (75.4 KB) - added by ramiy 13 years ago.
17603.3.diff (633 bytes) - added by nacin 13 years ago.
comment-bubble-rtl.png (1.7 KB) - added by yoavf 13 years ago.
comment-bubble-dark-rtl.png (1.8 KB) - added by yoavf 13 years ago.
twentyeleven-rtl.diff (18.4 KB) - added by yoavf 13 years ago.
One more try - with renamed images instead of duplicates

Download all attachments as: .zip

Change History (48)

@rasheed
13 years ago

#1 @ocean90
13 years ago

  • Cc lancewillett iandstewart added
  • Keywords needs-patch added; rtl-feedback removed
  • Milestone changed from Awaiting Review to 3.2

@ryan
13 years ago

#2 follow-up: @ryan
13 years ago

17603.2.diff adds an rtl class to the admin body and makes use of that in theme-options.css. There's little point in a separate -rtl.css for as little extra css as this requires.

@ryan
13 years ago

body.rtl

#3 @ryan
13 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [18125]:

Add rtl class to the admin body if is_rtl(). RTL fixes for twentyeleven theme-options.php. Props rasheed. fixes #17603

#4 in reply to: ↑ 2 ; follow-up: @ramiy
13 years ago

Replying to ryan:

There's little point in a separate -rtl.css for as little extra css as this requires.

What if we will have to make more RTL changes on this page? many more changes? separate rtl css is the best way to do this.


ooo, and i upgraded to wordpress ‎3.2-beta2-18085. the problem is not fixed. (using hebrew wordpress with ie8). i can upload a screenshot if you want.

#5 in reply to: ↑ 4 ; follow-up: @azaozz
13 years ago

Replying to ramiy:

What if we will have to make more RTL changes on this page?..

Actually was thinking we can combine all *-rtl.css files into one and load it on every screen. Don't see a point in having so many small separate files. This will make page loading a bit faster and simplify script-loader.

#6 in reply to: ↑ 5 @ramiy
13 years ago

Replying to azaozz:

we can combine all *-rtl.css files into one and load it on every screen.

This will make page loading a bit faster and simplify script-loader.

It's a good idea but you have to make sure that xxx.css will have only LTR rules and xxx-rtl.css will have only RTL rules.


I updated to wp-‎3.2-beta2-18145, the [18125] changeset did not fixed the problem.

@ramiy
13 years ago

#7 @ramiy
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#8 follow-up: @dd32
13 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

This was indeed fixed, you however most likely have an out of date TwentyEleven installation. Due to a bug in the upgrade code, it looks like Twenty Eleven didn't get updated when you updated Core.

If you try on a new installation, or delete the twentyeleven theme and then update again, you should find this fixed.

#9 in reply to: ↑ 8 @ramiy
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to dd32:

This was indeed fixed, you however most likely have an out of date TwentyEleven installation. Due to a bug in the upgrade code, it looks like Twenty Eleven didn't get updated when you updated Core.

If you try on a new installation, or delete the twentyeleven theme and then update again, you should find this fixed.

dd32, i deleted the twentyeleven theme and then updated again, the problem not fixed.

#10 @nacin
13 years ago

We can't mirror-image the images in an RTL form. Is there a problem with what you've circled? They seem correct -- "Content on left" has the content on left, and vice versa.

#11 @ramiy
13 years ago

Hi nacin,

  • Color Scheme - Need to be aligned to right.
  • Layout - Align to right. And about the pictures, in my language it's wrong. We can do something like this:
if ( !is_rtl() ) {
   echo 'sidebar-content.png';
} else {
   echo 'content-sidebar.png';
}

#12 @nacin
13 years ago

Rather than consider the photos wrong, let's consider the text wrong (you replaced left with right), and therefore also the theme's RTL styling wrong.

Get what I mean? What if we did no flipping at all? At most, we can then have it default to sidebar-content.

#13 @ramiy
13 years ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

guys, i re-upgraded the theme using ftp and seems like it's working ok.

#14 @ramiy
13 years ago

sorry for the inconvenience.

#15 follow-up: @nacin
13 years ago

I imagine on install it defaults to content-sidebar, while in RTL it should default to sidebar-content. Let's fix that.

@nacin
13 years ago

#16 follow-up: @nacin
13 years ago

  • Keywords has-patch added

Note that patch will not move cleanly to WP.com, where a user can be in RTL but a blog may not.

#17 in reply to: ↑ 15 @ramiy
13 years ago

Replying to nacin:

I imagine on install it defaults to content-sidebar, while in RTL it should default to sidebar-content. Let's fix that.

it's a good idea.

#18 in reply to: ↑ 16 @yoavf
13 years ago

Replying to nacin:
This works. I'm just a little bit troubled by the semantics in the code, since it's not really sidebar-content on rtl, it's still content-sidebar, but RTL :)

#19 @yoavf
13 years ago

Actually, Twenty Eleven RTL needs a bit of work (which I'm doing right now).
I think it's going to affect this - so better wait just a bit.

#20 @nacin
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#21 @ramiy
13 years ago

  • Summary changed from Theme options page (Arabic RTL) to TwentyEleven theme options page (RTL)

#22 @nacin
13 years ago

  • Summary changed from TwentyEleven theme options page (RTL) to Twenty Eleven theme options page (RTL)

It was fine as is, but note the space. :-)

#23 @yoavf
13 years ago

twentyeleven-rtl.diff updates the rtl.css file for Twenty Eleven.
It also adds proper RTL for the columns, making it easier to semantically reverse the images in the theme options.

The "Content on left/right" strings should be translated to "content on right/left" in RTL languages (we can add a comment for that string).

#24 @yoavf
13 years ago

The rtl.css is better than before, but still needs testing.
Asked a few people to do that now, I'll update in a bit if I get any feedback.

#25 follow-up: @nacin
13 years ago

Given that these images are the same, I don't think we need to add them multiple times.

That said, sidebar-content is just that -- sidebar on the left, content on the right. And vice versa. My patch wasn't very semantic, but it makes the code a lot less confusing. (We could also re-arrange them so sidebar-content comes first in the view in RTL.) Thoughts?

#26 in reply to: ↑ 25 ; follow-up: @yoavf
13 years ago

That said, sidebar-content is just that -- sidebar on the left, content on the right. And vice versa. My patch wasn't very semantic, but it makes the code a lot less confusing. (We could also re-arrange them so sidebar-content comes first in the view in RTL.) Thoughts?

Not quite right.
When you're in a rtl language mode, and you say sidebar content, you expect sidebar on the right, content on the left.

Maybe we should rename the images to say that explicitly, then we don't need the rtl versions, and can just switch the positions.
ie: sidebar-right-content-left.png / content-right-sidebar-left.png

#27 @yoavf
13 years ago

(the last version of twentyeleven-rtl.diff doesn't change that, just a small css update over the previous version)

#28 in reply to: ↑ 26 @nacin
13 years ago

Replying to yoavf:

Not quite right.
When you're in a rtl language mode, and you say sidebar content, you expect sidebar on the right, content on the left.

But content-sidebar and sidebar-content is just how the data is stored and the images are named. The strings themselves can properly reflect what's going on. Two options: Change two strings and a default value, or completely hack up the theme-options page, create duplicate CSS reversing the meaning of the classes in rtl.css, etc.

I don't mind the latter, but I want to make sure we're doing it for practical reasons rather than theoretical ones.

#29 @ocean90
13 years ago

yoavf: There is a typo in your patch. s/sans-serifl/sans-serif;/

#30 @yoavf
13 years ago

Just to bring this thread up to date following an IRC discussion -

@nacin said:

I'm wondering why we need to switch all of it though
It seems like a waste of energy

@yoavf said:

It's not - because when an rtl developer sees a class that says sidebar-content - he expects the sidebar on the right

@nacin said:

y'all are crazy

:)

#31 @iandstewart
13 years ago

If all "y'all" RTL developers expect sidebar-content to mean a sidebar on the right I'm all for switching out images and flopping the layout. :)

#32 @ramiy
13 years ago

in our language when we say 'sidebar-content', we start from right - first sidebar and then content. in english you start from left - first sidebat and them content.

#33 @ramiy
13 years ago

The best sollution is to change the text "content on left" to "content - sidebar", but it's not sounds good.

#34 @yoavf
13 years ago

The latest patch assumes:
wp-content/themes/twentyeleven/inc/images/content-sidebar.png renamed into
wp-content/themes/twentyeleven/inc/images/content-left.png
and
wp-content/themes/twentyeleven/inc/images/sidebar-content.png renamed into
wp-content/themes/twentyeleven/inc/images/content-right.png

This should make it all right - the same classes are used with both RTL and LTR, with the expected results, and no duplicate images.

@yoavf
13 years ago

One more try - with renamed images instead of duplicates

#35 @azaozz
13 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

I'm kind of lost here. Many themes let you add left or right sidebar, or both. What's the difference for RTL languages?

This ticket seems to be about the theme options page needing couple small fixes that have been made in the meantime. Don't see why it should stay open.

#36 follow-up: @yoavf
13 years ago

Andrew, I piggybacked on this ticket ro resolve a larger issue.
I can open another ticket but in any case - twenty eleven RTL is currently way off ltr - and needs an update.

#37 @nacin
13 years ago

Go ahead yoavf and set it for 3.2.

#38 @yoavf
13 years ago

Continued in #17882

#39 in reply to: ↑ 36 @azaozz
13 years ago

Replying to yoavf:

Andrew, I piggybacked on this ticket to resolve a larger issue.

Much better in a new ticket IMO :)

#40 @Anton Torvald
12 years ago

Nothing.

Last edited 12 years ago by ocean90 (previous) (diff)
Note: See TracTickets for help on using tickets.