WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#17603 closed enhancement (fixed)

Twenty Eleven theme options page (RTL)

Reported by: rasheed Owned by: 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 4 years ago.
17603.diff (938 bytes) - added by ryan 4 years ago.
17603.2.diff (1.1 KB) - added by ryan 4 years ago.
body.rtl
wp32rc1.PNG (75.4 KB) - added by ramiy 4 years ago.
17603.3.diff (633 bytes) - added by nacin 4 years ago.
comment-bubble-rtl.png (1.7 KB) - added by yoavf 4 years ago.
comment-bubble-dark-rtl.png (1.8 KB) - added by yoavf 4 years ago.
twentyeleven-rtl.diff (18.4 KB) - added by yoavf 4 years ago.
One more try - with renamed images instead of duplicates

Download all attachments as: .zip

Change History (48)

@rasheed4 years ago

comment:1 @ocean904 years ago

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

@ryan4 years ago

comment:2 follow-up: @ryan4 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.

@ryan4 years ago

body.rtl

comment:3 @ryan4 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

comment:4 in reply to: ↑ 2 ; follow-up: @ramiy4 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.

comment:5 in reply to: ↑ 4 ; follow-up: @azaozz4 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.

comment:6 in reply to: ↑ 5 @ramiy4 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.

@ramiy4 years ago

comment:7 @ramiy4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:8 follow-up: @dd324 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.

comment:9 in reply to: ↑ 8 @ramiy4 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.

comment:10 @nacin4 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.

comment:11 @ramiy4 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';
}

comment:12 @nacin4 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.

comment:13 @ramiy4 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.

comment:14 @ramiy4 years ago

sorry for the inconvenience.

comment:15 follow-up: @nacin4 years ago

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

@nacin4 years ago

comment:16 follow-up: @nacin4 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.

comment:17 in reply to: ↑ 15 @ramiy4 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.

comment:18 in reply to: ↑ 16 @yoavf4 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 :)

comment:19 @yoavf4 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.

comment:20 @nacin4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:21 @ramiy4 years ago

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

comment:22 @nacin4 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. :-)

@yoavf4 years ago

comment:23 @yoavf4 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).

comment:24 @yoavf4 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.

comment:25 follow-up: @nacin4 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?

comment:26 in reply to: ↑ 25 ; follow-up: @yoavf4 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

comment:27 @yoavf4 years ago

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

comment:28 in reply to: ↑ 26 @nacin4 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.

comment:29 @ocean904 years ago

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

comment:30 @yoavf4 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

:)

comment:31 @iandstewart4 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. :)

comment:32 @ramiy4 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.

comment:33 @ramiy4 years ago

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

comment:34 @yoavf4 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.

@yoavf4 years ago

One more try - with renamed images instead of duplicates

comment:35 @azaozz4 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.

comment:36 follow-up: @yoavf4 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.

comment:37 @nacin4 years ago

Go ahead yoavf and set it for 3.2.

comment:38 @yoavf4 years ago

Continued in #17882

comment:39 in reply to: ↑ 36 @azaozz4 years ago

Replying to yoavf:

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

Much better in a new ticket IMO :)

comment:40 @Anton Torvald4 years ago

download ebookhood new moon download ebook script game ebook free

Version 0, edited 4 years ago by Anton Torvald (next)
Note: See TracTickets for help on using tickets.