WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#17882 closed defect (bug) (fixed)

Twenty Eleven RTL update

Reported by: yoavf Owned by: azaozz
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.2
Component: RTL Keywords: has-patch commit
Focuses: Cc:

Description

Twenty Eleven RTL css needs quite a few updates and addition of missing styles.
Also, following the discussion in #17603, I'd like the layout classes to be RTL compatible as well (ie. '.sidebar-content' should mean that the sidebar is on the right in RTL mode)

For that to happen, the theme options page need to be tweaked a little bit to make sure the proper image is shown.

The easiest way to do that is to rename
wp-content/themes/twentyeleven/inc/images/content-sidebar.png into
wp-content/themes/twentyeleven/inc/images/content-left.png
and
wp-content/themes/twentyeleven/inc/images/sidebar-content.png into
wp-content/themes/twentyeleven/inc/images/content-right.png

See attached patch.

Attachments (4)

twentyeleven-rtl.diff (18.4 KB) - added by yoavf 3 years ago.
17882.patch (4.3 KB) - added by azaozz 3 years ago.
Renamed the images too, they won't show until the patch is committed.
17882-2.patch (2.6 KB) - added by azaozz 3 years ago.
17882-3.patch (18.7 KB) - added by yoavf 3 years ago.

Download all attachments as: .zip

Change History (18)

yoavf3 years ago

comment:1 ocean903 years ago

  • Keywords has-patch added

comment:2 in reply to: ↑ description ; follow-up: azaozz3 years ago

I'm still lost here. Seems you want to make Left to mean Right and Right to mean Left when coding.

Replying to yoavf:

... I'd like the layout classes to be RTL compatible as well (ie. '.sidebar-content' should mean that the sidebar is on the right in RTL mode)

I'm against it because:

  • this is not a translatable string
  • this is code, i.e. not user-readable
  • I can understand the desire to make the theme fully RTL compliant, but setting small parts of the code to have the opposite meaning seems very confusing.

Unfortunately don't think there are any right-to-left coding languages (C, PHP, JS, etc.). All are left-to-right and use English words. So all "programers" need to understand a little English in order to learn to code.

Was wondering, how do RTL language speakers write code? Do they use an RTL text editor and see something like:

                                          one-column .commentlist > li.comment { 
                                                                 margin-left: 0;
                                                            margin-right: 102px;
                                                                               }

But even then swapping Left and Right for one class only would be confusing.

Version 0, edited 3 years ago by azaozz (next)

comment:3 in reply to: ↑ 2 yoavf3 years ago

Replying to azaozz:

I'm still lost here. Seems you want to make Left to mean Right and Right to mean Left when coding.

No - exactly the opposite.
Right should be right, and left left.
But sidebar-content means different things to rtl/ltr developers.

There are no changes to right and left in my patch - I simply want to abstract it so that it's not direction dependent, but rather content dependent, and everything should match the design spirit of the theme.

If the theme designer wants the sidebar on the right as a default for LTR languages, then it should be on the left for RTL languages. That's it.

  • I can understand the desire to make the theme fully RTL compliant, but setting small parts of the code to have the opposite meaning seems very confusing.

What has the opposite meaning?

Was wondering, how do RTL language speakers write code? Do they use an RTL text editor and see something like:

No :)

comment:4 yoavf3 years ago

To clarify a little:

When a class name says .alignright in style.css, you won't find a counterpart in rtl.css.
Since align right means just that - align to the right

but if there' something like

.sidebar-content{
float:left;
}

then on rtl.css it should be:

.sidebar-content{
float:right;
}

Because sidebar-content means "sidebar first, content next" and in RTL that means the sidebar should be on the right.

comment:5 follow-up: azaozz3 years ago

Replying to yoavf:

But sidebar-content means different things to rtl/ltr developers.

Thanks, much clearer now :)

As the theme has options to put the sidebar on the right or left, thinking we should rename .sidebar-content to .left-sidebar and .content-sidebar to .right-sidebar and be done with it. This would make it a lot clearer in both RTL and LTR cases IMO.

For example I read .sidebar-content as "the sidebar is on the left side of the content". In that terms I would expect it to be always on the left. However it can be read as "the sidebar is before the content" and expected to switch places in RTL.

comment:6 in reply to: ↑ 5 yoavf3 years ago

As the theme has options to put the sidebar on the right or left, thinking we should rename .sidebar-content to .left-sidebar and .content-sidebar to .right-sidebar and be done with it. This would make it a lot clearer in both RTL and LTR cases IMO.

I'm perfectly ok with that - but I think that patch would be a little wider at that point as it would require style.css changes and probably changes in the template files as well.

We'll also need to set the default option to .right-sidebar in RTL - so theme-options.php would still need to be patched.

azaozz3 years ago

Renamed the images too, they won't show until the patch is committed.

comment:7 yoavf3 years ago

17882.patch - looks good, and resolves the right-left problem, thanks azaozz.

Once this is committed, I'll refresh the rtl file some more (please leave the ticket open).

comment:8 nacin3 years ago

Let's get moving on those refreshes then.

comment:9 follow-up: nacin3 years ago

Uh, I was about to go forward with this, but this is going to tank any existing usage of Twenty Eleven. We need some sort of upgrade routine at least for RC2. This renaming thing is crazy. Let's just comment it for RTL folks.

comment:10 in reply to: ↑ 9 yoavf3 years ago

Replying to nacin:

Uh, I was about to go forward with this, but this is going to tank any existing usage of Twenty Eleven. We need some sort of upgrade routine at least for RC2. This renaming thing is crazy. Let's just comment it for RTL folks.

I don't mind going either way but in this case I think twentyeleven-rtl.diff does the trick.
Existing style.css doesn't change, rtl styles are added where necessary, and the change to the theme options is rather minor.

azaozz3 years ago

comment:11 azaozz3 years ago

17882-2.patch is similar to 17882.patch except it doesn't change options names so no need to reset the options.

yoavf3 years ago

comment:12 yoavf3 years ago

17882-3.patch combines azaozz's 17882-2.patch with a complete refresh of twenty eleven rtl.
Good to go if you ask me :-)

comment:13 nacin3 years ago

  • Keywords commit added

+1 on 17882-3.patch from azaozz and yoavf.

comment:14 azaozz3 years ago

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

In [18342]:

Twenty Eleven RTL update, props yoavf, fixes #17882

Note: See TracTickets for help on using tickets.