Ticket #7498 (closed enhancement: fixed)

Opened 3 years ago

Last modified 3 years ago

Revised RTL CSS for 2.6

Reported by: mani_monaj Owned by: anonymous
Priority: high Milestone: 2.6.1
Component: I18N Version:
Severity: normal Keywords: rtl, i18n tested has-patch
Cc:

Description

All *-rtl.css files for Wordpress (currently for 2.6 branch) has been re-coded and revised. This was part of wp-persian project. The new set of CSS files are compatible with FF 2 and 3, IE 6 and 7.

Attachments

wp-26-rtl-rewrite-patch.diff Download (31.6 KB) - added by mani_monaj 3 years ago.
wp-26-rtl-rewrite-patch-IR-and-IL.diff Download (31.9 KB) - added by RanYanivHartstein 3 years ago.
wp-26-rtl-rewrite-patch-IR-and-IL-nocreadit.diff Download (31.3 KB) - added by mani_monaj 3 years ago.
No credits …
wp-26-rtl-rewrite-patch-final-1.patch Download (31.4 KB) - added by mani_monaj 3 years ago.
Final patch for 2.6.1

Change History

  • Keywords tested has-patch added

This is a great patch, thanks a lot on behalf of any RTLer that hasn't already thanked you.

I tested it on FF3, IE7 and IE6 and noticed a few minor issues, I'm including a patch to address them:

  • Fonts re-set back to a more generic "sans-serif", so for every script and every platform the best/most common font is chosen. This was discussed in #6616.
  • Fixed peek-a-boo bug with sidemenu in IE6
  • Added white space before submenu, to align it with the main menu and other admin widgets
  • Centered media-upload window in IE
  • Fixed submenu in media-upload window in IE (it would get offset way out of view to the left)
  • Fixed install pages in IE6 (half the page would get hidden out of view to the right)

Apart from these changes, my patch is identical to yours.

There is one issue I didn't presume to address in my patch, but I do think it's worthy discussing - the credit issue. In your patch, all the RTL files are tagged by your team, with specific credit to one Navid Kashani.

While your work was of course noteworthy and deserving of credit, placing that credit in every file has the side effect of denying credit from any other developers and testers who worked on the RTL style sheets, my self included. Since there is no reasonable way to properly credit everyone who contributed to these styles, I think it would be best to avoid this practice all together, and commit these files without those lines - like all the other files in the WordPress source.

comment:2   ryan3 years ago

I strip attribution comments whenever I come across them. We handle attribution through the "Props" comments in the commit messages which are later extracted to create a Credits list. I'll make sure the person mentioned in the comments gets "Props".

I removed all credit lines, but the font issue is still under discussion. The current patch is still based on Ran's proposed "Sans Serif" fonts, however Iranian users rather use of Tahoma font. We are working with Ran to reach a solution for 2.6.1;

Quoting from my correspondence with mani_monaj regarding the font issue:

The font issue is not such a big deal at all [...] we can definitely live with it, and even if we can't, I'd sooner make a local override just in our distribution than force our choices on everybody else.

Regarding a long term solution, how about a third level of style sheet, unique to each locale? For e.g., for the ie.css, when rtl is activated the ie-rtl.css style sheet would get loaded, and if for e.g. the he_IL locale is set, an extra ie-he_IL.css style sheet would also get loaded.

To simplify things, this could probably be one global locale.css style sheet that will get loaded in every admin page, instead of a locale-specific version for every different style sheet.

Technically, this could be probably already be achieved using the locale.php file to add overriding styles in every admin page, but a permanent solution would be easier to use.

comment:5   ryan3 years ago

I've thought about both approaches for 2.7. Using $locale.php might not be so bad if we add some API to streamline things. That would save adding more file_exists checks. Either approach is fine by me.

So the latest patch is okay with everyone for 2.6.1?

Ran and I discussed the issues and I'll submit the final patch now. This patch is the revised version of the first patch including Ran's proposed changes except the font issue.

Final patch for 2.6.1

comment:7   ryan3 years ago

There were some conflicts when applying the patch to trunk. Patch is clean with 2.6. Committing.

comment:8   ryan3 years ago

(In [8631]) RTL CSS updates from Navid Kashani, RanYanivHartstein, mani_monaj, and the WP Persian team. see #7498

comment:9 follow-up: ↓ 10   ryan3 years ago

That's for 2.6.1. Leaving the ticket open for trunk patch.

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

Replying to ryan:

That's for 2.6.1. Leaving the ticket open for trunk patch.

Thank you Ryan. In our discussion with Ran, we talked about a work group for Wordpress RTL issues. I think we (the work group) can work together for further changes. Can I post the proposal here?

Sure. Or in Codex if you like.

(In [8632]) RTL CSS updates from Navid Kashani, RanYanivHartstein, mani_monaj, and the WP Persian team. see #7498. Partial patch for trunk

comment:13 follow-up: ↓ 14   ryan3 years ago

ie-rtl.css and rtl.css patches didn't apply, but I committed the other files for trunk. Can anyone give those a look?

comment:14 in reply to: ↑ 13   mani_monaj3 years ago

Replying to ryan:

ie-rtl.css and rtl.css patches didn't apply, but I committed the other files for trunk. Can anyone give those a look?

We are now working on CSS files in trunk. I'll post the results here.

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

As 2.6.1 has now gone I am going to close this as fixed.

Please raise a new ticket for the remaining trunk fixes.

(In [8663]) Sync trunk RTL styles with 2.6 branch. see #7498

Note: See TracTickets for help on using tickets.