WordPress.org
Get WordPress

Make WordPress Core

Opened 15 months ago

Closed 13 months ago

Last modified 13 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
15 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
15 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
15 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 15 months ago by audrasjb (previous) (diff)

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

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

#5 @sabernhardt
15 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
15 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
13 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 13 months ago by sabernhardt (previous) (diff)
Note: See TracTickets for help on using tickets.