WordPress.org

Make WordPress Core

Opened 5 years ago

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

Download all attachments as: .zip

Change History (48)

@rasheed
5 years ago

#1 @ocean90
5 years ago

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

@ryan
5 years ago

#2 follow-up: @ryan
5 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
5 years ago

body.rtl

#3 @ryan
5 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
5 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
5 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
5 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
5 years ago

#7 @ramiy
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#8 follow-up: @dd32
5 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
5 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
5 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
5 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
5 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
5 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
5 years ago

sorry for the inconvenience.

#15 follow-up: @nacin
5 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
5 years ago

#16 follow-up: @nacin
5 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
5 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
5 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
5 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
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#21 @ramiy
5 years ago

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

#22 @nacin
5 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
5 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
5 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
5 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
5 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
5 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
5 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
5 years ago

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

#30 @yoavf
5 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
5 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
5 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
5 years ago

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

#34 @yoavf
5 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
5 years ago

One more try - with renamed images instead of duplicates

#35 @azaozz
5 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
5 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
5 years ago

Go ahead yoavf and set it for 3.2.

#38 @yoavf
5 years ago

Continued in #17882

#39 in reply to: ↑ 36 @azaozz
5 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
4 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.