WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 14 months ago

Last modified 12 months ago

#23550 closed defect (bug) (fixed)

Twenty Thirteen: add RTL support

Reported by: lancewillett Owned by:
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Bundled Theme Keywords:
Focuses: Cc:

Description

Probably OK to wait until further in the cycle, closer to freeze -- but adding it so we don't forget.

Attachments (5)

23550.diff (1.5 KB) - added by DrewAPicture 14 months ago.
23550.2.diff (6.2 KB) - added by maor 14 months ago.
23550.3.diff (22.5 KB) - added by obenland 14 months ago.
23550.4.diff (925 bytes) - added by obenland 14 months ago.
23550.5.diff (2.2 KB) - added by maor 12 months ago.

Download all attachments as: .zip

Change History (27)

DrewAPicture14 months ago

comment:1 DrewAPicture14 months ago

  • Keywords has-patch added; needs-patch removed

Noticed a few things testing RTL for something else, so I got the ball rolling with 23550.diff.

I opted to keep the same index structure as stlye.css just to keep straight what I was overriding. As @lancewillett said in the ticket summary, this can probably wait til freeze, but here's a start.

comment:2 maor14 months ago

  • Cc maorhaz@… added

I know it might be a bit too early at this point, but I was going to use Twenty Thirteen on my blog anyway, so I worked a bit on the RTL stylesheet.

23550.2.diff extends Drew's initial patch. There's still room for improvement, but I have covered most of the RTL rules (incl. responsive design). As you may notice, I've also changed the functions.js file, since the widgets area in the footer is controlled from there (Twenty Thirteen is using jQuery Masonry, which does not enable RTL support by default but instead, offers a manual setting/switch for RTL pages - "isRTL").

Here's a quick snapshot of how it looks after 23550.2.diff is applied.
http://i.imgur.com/eTcMOJb.png

maor14 months ago

comment:3 follow-up: obenland14 months ago

maor: Man I was so hoping to see Twenty Thirteen on a site with content in Hebrew!

comment:4 in reply to: ↑ 3 maor14 months ago

Replying to obenland:

maor: Man I was so hoping to see Twenty Thirteen on a site with content in Hebrew!

Totally forgot to add a link. Here you go!
http://wpm.co.il/staging/trunk

comment:5 follow-ups: lancewillett14 months ago

@maor Was the JavaScript change intentional? That should probably go in a different ticket / patch, please.

comment:6 in reply to: ↑ 5 maor14 months ago

Replying to lancewillett:

@maor Was the JavaScript change intentional? That should probably go in a different ticket / patch, please.

Yes, it was intentional. It's quite impossible to make the theme work under RTL environments without setting jQuery Masonry to RTL mode. Should I go ahead and create a new ticket for that, or is there an open one already?

comment:7 in reply to: ↑ 5 lancewillett14 months ago

Replying to lancewillett:

@maor Was the JavaScript change intentional? That should probably go in a different ticket / patch, please.

Oh, here is fine -- I didn't see the RTL line, I just saw the body selector improvements. :) Cool. That's fine.

comment:8 lancewillett14 months ago

@maor -- If you're around, jump into #wordpress-dev IRC Tue/Thu for Twenty Thirteen office hours, and thank you for your help on this!

http://make.wordpress.org/core/tag/twentythirteen/ and in the sidebar is the office hours link and times.

comment:9 maor14 months ago

Perfect! It's all my pleasure!

It's a bummer I missed the office hours for today. I'll do my best to join next week.

comment:10 lancewillett14 months ago

In 23643:

Twenty Thirteen: first pass at RTL CSS and JS support. Props DrewAPicture and maor, see #23550.

comment:11 maor14 months ago

Oops! Seems like I missed the dot within the .is( ... ) conditional.

isRTL: body.is( 'rtl' ) ? true : false

Should be:

isRTL: body.is( '.rtl' ) ? true : false

Sorry about that!

comment:12 lancewillett14 months ago

In 23644:

Twenty Thirteen: RTL JS support, fix class selector, see #23550.

comment:13 lancewillett14 months ago

For the isRTL check, we might not need the tertiary notation.

Also, in other places in core WP this is the method:

isRTL: !! ( 'undefined' != typeof isRtl && isRtl )

From http://core.trac.wordpress.org/browser/trunk/wp-admin/js/custom-header.js?rev=23009#L12

comment:14 obenland14 months ago

Other places would be the admin, We would have to define the variable ourselves then. I'm fine with the is() check, though

Version 0, edited 14 months ago by obenland (next)

comment:15 lancewillett14 months ago

In 23645:

Twenty Thirteen: simpler isRTL check for masonry, props obenland. See #23550.

obenland14 months ago

comment:16 obenland14 months ago

  • Keywords needs-testing added
  • Version set to trunk

23550.3.diff introduces the second pass on RTL and some minor style.css cleanup that I discovered while examining it in the RTL process. Needs testing!!

Last edited 14 months ago by lancewillett (previous) (diff)

comment:17 lancewillett14 months ago

In 23674:

Twenty Thirteen: second pass at RTL support and minor style.css fixes, props obenland. See #23550.

obenland14 months ago

comment:18 obenland14 months ago

Unfortunately I misinterpreted the purpose of three content attributes, which led to bugs in r23674. 23550.4.diff addresses that.

comment:19 lancewillett14 months ago

In 23687:

Twenty Thirteen: revert three changes to content attributes, props obenland. See #23550.

comment:20 lancewillett14 months ago

  • Keywords has-patch needs-testing removed
  • Resolution set to fixed
  • Status changed from new to closed

Discussing in core dev chat today, we're closing this main ticket. Lance is asking @maor and @yoav to do thorough testing for us, and they'll open new issues for new bugs.

maor12 months ago

comment:21 follow-up: maor12 months ago

Hey y'all! It's been a while...

I went over the theme again this weekend and was only able to find 2 RTL issues. Both are minor.

Let me explain a bit about 23550.5.diff.

Both fixes have to do with the pagination arrows that are showing up on attachment pages and on single posts. The thing is that the direction of the arrows should be flipped on RTL sites, otherwise it'll look quite weird (imagine this - http://note.io/10NbNym).

This is how the single post view looks with the patch applied. Notice that the links have been flipped to match the RTL "visual language" http://note.io/10CVgmj

There is the $text parmeter that is being passed on to all of these functions (previous_post_link, next_post_link, previous_image_link and next_image_link). There was one case where I couldn't use sprintf() to place the proper arrow direction because of some text that looks like a sprintf tag (Where it says %title -- '<span class="meta-nav">&larr;</span> %title'). So instead I used a if ( is_rtl() ) : ... else : ... endif; conditional.

There might be a better way to do it. I'd like to hear what you guys think about it. If I wasn't clear enough, I can try explaining it again.

Cheers!

comment:22 in reply to: ↑ 21 lancewillett12 months ago

Replying to maor:

There might be a better way to do it. I'd like to hear what you guys think about it. If I wasn't clear enough, I can try explaining it again.

Thanks Maor, for the look and your notes.

The better way to handle this is in the translation files for Hebrew (or other RTL language). There, you'd flip the arrow as needed—not in the theme source.

Note: See TracTickets for help on using tickets.