Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#43466 closed enhancement (wontfix)

Add ltr admin body class

Reported by: birgire's profile birgire Owned by:
Milestone: Priority: low
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: rtl Cc:

Description (last modified by birgire)

In wp-admin/admin-header.php we have:

if ( is_rtl() ) {
        $admin_body_class .= ' rtl';
}

added in [18125].

But I wonder if we should have both classes available:

$admin_body_class .= is_rtl() ? ' rtl' : ' ltr';

e.g. to help CSS or JS selectors target ltr only.

Attachments (2)

43466.diff (5.0 KB) - added by nielslange 6 years ago.
43466.2.diff (4.7 KB) - added by nielslange 6 years ago.

Download all attachments as: .zip

Change History (16)

#1 @birgire
6 years ago

  • Description modified (diff)

#2 @SergeyBiryukov
6 years ago

If we go with it, the same should be done in these files as well for consistency:

wp-admin/includes/template.php
wp-admin/customize.php
wp-admin/install.php
wp-admin/setup-config.php
wp-includes/post-template.php
wp-login.php

#3 in reply to: ↑ description @SergeyBiryukov
6 years ago

Replying to birgire:

e.g. to help CSS or JS selectors target ltr only.

Do you have a use case that would be simpler to achieve with the ltr class?

#4 @birgire
6 years ago

@SergeyBiryukov Thanks for checking out the other admin files.

I see that there's also no dir="ltr" for the <html> tag, just dir="rtl":

$dir_attr = '';
if ( is_rtl() ) {
	$dir_attr = ' dir="rtl"';        
}

in wp-includes/load.php:

I was just speculating about various possible options to avoid complicating things with PHP and is_rtl() in inline admin styling.

Here's an example from the Health Check plugin, related to #39165 src:

 <span style="display: block; width: 100%; text-align: <?php echo ( is_rtl() ? 'left' : 'right' ); ?>"> 
     <a href="#system-information-table-of-contents" class="debug-toc"><?php esc_html_e( 'Return to table of contents' ); ?></a> 
 </span> 

Here's an example from Hello Dolly plugin (1.6):

// We need some CSS to position the paragraph
function dolly_css() {
        // This makes sure that the positioning is also good for right-to-left languages
        $x = is_rtl() ? 'left' : 'right';

        echo "
        <style type='text/css'>
        #dolly {
                float: $x;
                padding-$x: 15px;
                padding-top: 5px;
                margin: 0;
                font-size: 11px;
        }
        </style>
        ";
}

This could be written in pure (enqueued) CSS, using .rtl to help override float and padding directions.

There are also tools that can auto-generate the rtl stylesheet (like core is using).

There're also the textleft, textright, alignleft and alignright classes.

Another approach could be to not use an override, but use both .rtl and .ltr for the float and padding directions. But if that's against best practices then we shouldn't.

Last edited 6 years ago by birgire (previous) (diff)

#5 @nielslange
6 years ago

@birgire: What would be the benefit of adding .ltr to the body class? If I understand you correctly, the benefit would be to help CSS and JS to target specific elements directly, correct? Would the definition body::not('.rtl'), in case of CSS, not be sufficient enough?

Last edited 6 years ago by nielslange (previous) (diff)

#6 follow-up: @birgire
6 years ago

@nielslange yes, thanks for reminding me of the :not() CSS pseudo-class.

I had totally forgotten about it ;-)

Probably because I've used it so rarely

I wonder if that could be true for some other plugin authors as well? ;-)

ps: I noticed that Drupal 8.5 uses dir="ltr" on the <html> admin tag, so an alternative html[dir="ltr"] would be possible there. But WP only uses dir="rtl".

Last edited 6 years ago by birgire (previous) (diff)

@nielslange
6 years ago

@nielslange
6 years ago

#7 @nielslange
6 years ago

  • Focuses rtl added
  • Keywords has-patch needs-testing added; good-first-bug removed

#8 @nielslange
6 years ago

@birgire I created a patch for this issue. The worse thing that can happen is that the patch gets refused, right? 😉

#9 in reply to: ↑ 6 @nielslange
6 years ago

Replying to birgire:

@nielslange yes, thanks for reminding me of the :not() CSS pseudo-class.

I had totally forgotten about it ;-)

Probably because I've used it so rarely

In some cases, e.g. as long as ltr hasn't been implemented (or merged), :not() comes quite handy.

#10 follow-up: @birgire
6 years ago

@nielslange yes thanks for the patch and the :not() reminder ;-)

ps: the ternary operator ?: could be an alternative shorter version for if/else.

Last edited 6 years ago by birgire (previous) (diff)

#11 in reply to: ↑ 10 @nielslange
6 years ago

Replying to birgire:

ps: the ternary operator ?: could be an alternative shorter version for if/else.

As the conventional if else statement had been used on several occasions I decided not to use the ternary operator to avoid mixed coding styles.

#12 @pento
5 years ago

I don't think we need to add this for the dir attributes, as ltr is the default value for that attribute.

As for the class, it seems like :not( '.rtl' ) is a perfectly valid selector, if you ever need that. The more common pattern is to write the CSS to work in LTR, then add a smaller file of RTL rules that override the LTR rules where necessary. The examples in comment #4 should really be making use of that pattern.

#13 follow-up: @nielslange
5 years ago

@pento: Thanks for your feedback!

@birgire: Guess we can close this issue then, can't we?

#14 in reply to: ↑ 13 @birgire
5 years ago

  • Keywords needs-testing removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Replying to nielslange:

@pento: Thanks for your feedback!

@birgire: Guess we can close this issue then, can't we?

@nielslange yes, as we have the :not( '.rtl' ) workaround,

and thanks for looking into this.


Note: See TracTickets for help on using tickets.