WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 3 months ago

#43466 new enhancement

Add ltr admin body class

Reported by: birgire Owned by:
Milestone: Awaiting Review Priority: low
Severity: normal Version:
Component: Administration Keywords: has-patch needs-testing
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 3 months ago.
43466.2.diff (4.7 KB) - added by nielslange 3 months ago.

Download all attachments as: .zip

Change History (13)

#1 @birgire
4 months ago

  • Description modified (diff)

#2 @SergeyBiryukov
4 months 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
4 months 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
4 months 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 4 months ago by birgire (previous) (diff)

#5 @nielslange
3 months 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 3 months ago by nielslange (previous) (diff)

#6 follow-up: @birgire
3 months 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 3 months ago by birgire (previous) (diff)

@nielslange
3 months ago

@nielslange
3 months ago

#7 @nielslange
3 months ago

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

#8 @nielslange
3 months 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
3 months 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
3 months 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 3 months ago by birgire (previous) (diff)

#11 in reply to: ↑ 10 @nielslange
3 months 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.

Note: See TracTickets for help on using tickets.