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

[letraceursnork]

./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?

[sabernhardt]

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)

[letraceursnork]

@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

[audrasjb]

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! :)

[SergeyBiryukov]

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

[sabernhardt]

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.

[letraceursnork]

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

[sabernhardt]

#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.)