WordPress.org
Get WordPress

Make WordPress Core

Opened 9 months ago

Closed 8 months ago

Last modified 8 months ago

#58946 closed enhancement (duplicate)

Specificity of _admin_bar_bump_cb() echo and violation of MVC by that function

Reported by: letraceursnork's profile letraceursnork Owned by:
Milestone: Priority: normal
Severity: minor Version: 6.2.2
Component: Toolbar Keywords:
Focuses: Cc:

Description

./wp-icnludes/admin-bar.php:1219

What in the world is that?

<?php
/**
 * Prints default admin bar callback.
 *
 * @since 3.1.0
 */
function _admin_bar_bump_cb() {
        $type_attr = current_theme_supports( 'html5', 'style' ) ? '' : ' type="text/css"';
        ?>
<style<?php echo $type_attr; ?> media="screen">
        html { margin-top: 32px !important; }
        @media screen and ( max-width: 782px ) {
                html { margin-top: 46px !important; }
        }
</style>
        <?php
}

First of all, why !important?
Second of all, why 782px exactly? I couldn't find any framework that considering 782px as a breakpoint, the most popular sm breakpoint (between mobiles and tablets) is 768px
Then we come to hardcoded 32px and 46px. I just wanted to say about CSS-properties to handle this situation, when I found --wp-admin--admin-bar--height property inside html style and it customizes just like that - 32px and 46px on <782px. So, why not using this?

About MVC violation: what is <html>-layout doing in "functions"-like file? This one should describe functions and internal logic, not layouts, we have View-layer for that, or at least sprintf()

At last, what does this function do anyway? Just in a nutshell, I believe, this one "customizes" #wpadminbar for those themes who doesn't have its own add_filter() customization for it. Well, why not just include the CSS-file directly?

Change History (7)

#1 @sabernhardt
9 months ago

  • Component changed from General to Toolbar
  • Keywords needs-patch removed
  • Version changed from 6.2.2 to 3.1

Hi and thanks for the ticket!

The toolbar has fixed (or absolute) positioning, so a top margin can prevent it from covering front-end content.

History:

  • #15592 created the callback function
  • #15851 added the !important "nuclear option"
  • #16222 switched padding to margin
  • #25858 set a 782px breakpoint
  • #52623 created the CSS custom property, without using it in the callback (#54032 could change that, but I would prefer to wait a bit longer)

#2 @letraceursnork
9 months ago

@sabernhardt well, that's great, but how about fix it a little bit? Switching breakpoint to more convenient 768px, use separate CSS-file instead of <style>-tag layouts in PHP file? I can handle other CSS-related stuff in other tickets after that

#3 @audrasjb
9 months ago

  • Component changed from Toolbar to General
  • Version changed from 3.1 to 6.2.2

Hello and thanks for the ticket,

I'm not a CSS frameworks specialist, but I don't see any point in using 782px over 768px for this breakpoint. Let's just us what does fit with the best with our interface. In a real responsive design, breakpoints shouldn't adapt to any device. They should just adapt to the interface to make it responsive (= able to adapt to any situation).

But that's only how I _think_ breakpoint should be used. This is of course an open discussion :)

Last point: @letraceursnork please consider adapting a bit your speech, as formal statements are always better (and more professional, inclusive, and so on) than things like "What in the world is that?".
Thank you! :)

Last edited 9 months ago by audrasjb (previous) (diff)

#4 in reply to: ↑ description @SergeyBiryukov
9 months ago

Just noting that the 782px breakpoint is extensively used across the whole admin interface, not only in the admin bar.

#5 @sabernhardt
9 months ago

Changing the breakpoint (throughout Core) would be a large project and a backward compatibility problem with many plugins and themes that have used max-width: 782px and/or min-width: 783px.

#6 @letraceursnork
9 months ago

Ok, so far we've managed to handle 782px breakpoint. Honestly, my two main points were usage of CSS-property and View-layer inside a PHP-function

#7 @sabernhardt
8 months ago

  • Component changed from General to Toolbar
  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

#58775 already proposes to revise both admin toolbar functions (among others), so I'll close as a duplicate of that ticket.

If/when the default callback should include the CSS custom property, #54032 will need a new patch. However, I am not ready to make recommend making that change yet. (Internet Explorer usage is down to about 0.4%, and its users should have received warnings of possible incompatibilities by now. Opera Mini has twice as many users, and switching to the variable could affect them too.)

Last edited 8 months ago by sabernhardt (previous) (diff)
Note: See TracTickets for help on using tickets.