WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 months ago

#25906 closed defect (bug) (fixed)

Twenty Fourteen: adjust fixed header margin for MP6 merge

Reported by: celloexpressions Owned by: lancewillett
Milestone: 3.8 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description

Now that MP6 is in core, we need to account for its admin bar size always, and can't rely on the mp6 body class (although, this will probably be an issue for a lot of sites, might be worth keeping the mp6 body class around so that themes can give the appropriate handling with the admin bar regardless of WordPress version).

Attachments (5)

25906.diff (408 bytes) - added by celloexpressions 5 months ago.
Always use the mp6 rule
25906.2.diff (1.0 KB) - added by SergeyBiryukov 5 months ago.
25906.3.diff (1.9 KB) - added by SergeyBiryukov 5 months ago.
25906.4.diff (444 bytes) - added by obenland 5 months ago.
25906.5.diff (563 bytes) - added by celloexpressions 5 months ago.
Add min-height to the admin bar, to ensure that it always covers the minimum gap we have set.

Download all attachments as: .zip

Change History (31)

celloexpressions5 months ago

Always use the mp6 rule

comment:1 follow-up: lancewillett5 months ago

I think for at least one more version we should keep both around. 3.6 and 3.7 users should be able to use Twenty Fourteen without a hitch, right?

comment:2 lancewillett5 months ago

  • Milestone changed from Awaiting Review to 3.8

comment:3 in reply to: ↑ 1 celloexpressions5 months ago

Replying to lancewillett:

I think for at least one more version we should keep both around. 3.6 and 3.7 users should be able to use Twenty Fourteen without a hitch, right?

I agree that that's ideal, but that would mean that we'd either need to do a version check or permanently add a core body class of mp6 along with the admin bar. This would also preserve compatibility with all of the themes that have been developed with something similar to this (an mp6 adjustment). I'm not sure what the merge team's plans are but it isn't there at the moment.

comment:4 follow-up: sabreuse5 months ago

Some discussion just a few minutes ago in wordpress-dev re: keeping the MP6 class: https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-11-11&sort=asc#m725609

Sounds like the plan is to keep the class (EDIT: actually, I'm not sure based on that discussion, as both keeping and dropping it are in the mix), but team 2014 should check in on the MP6 team discussion before deciding how to handle the admin bar one way or the other.

Last edited 5 months ago by sabreuse (previous) (diff)

comment:5 in reply to: ↑ 4 lancewillett5 months ago

Replying to sabreuse:

Some discussion just a few minutes ago in wordpress-dev re: keeping the MP6 class: https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-11-11&sort=asc#m725609

Sounds like the plan is to keep the class (EDIT: actually, I'm not sure based on that discussion, as both keeping and dropping it are in the mix), but team 2014 should check in on the MP6 team discussion before deciding how to handle the admin bar one way or the other.

Thanks for the discussion link. I think we should follow MP6 team's lead, as you say. Otherwise I don't think it's harmful to leave both selectors, back compat is a huge priority with WordPress. :)

comment:6 sabreuse5 months ago

Totally agree -- far better to have a class we don't (necessarily) need than pull it and need it later.

comment:7 follow-up: helen5 months ago

Not keeping the mp6 class name. It's non-semantic and sets a bad precedent for addition and maintenance of body classes. Maybe the real issue here is that we're dealing with a magic number because of having a fixed toolbar that conditionally shows?

comment:8 in reply to: ↑ 7 lancewillett5 months ago

Replying to helen:

Maybe the real issue here is that we're dealing with a magic number because of having a fixed toolbar that conditionally shows?

I'd say the real issue is that the toolbar height changed in core. ;)

comment:9 follow-up: helen5 months ago

Magic number, then. What would a long-term solution to this be? A function to return toolbar height? A reusable CSS class that has the top value defined? Something else? Because who knows what could change in the future.

comment:10 in reply to: ↑ 9 lancewillett5 months ago

Replying to helen:

Magic number, then. What would a long-term solution to this be? A function to return toolbar height? A reusable CSS class that has the top value defined? Something else? Because who knows what could change in the future.

Maybe a new body class value, introduced in 3.8, that wouldn't exist in older installs. Same in purpose as .mp6 but more meaningful and not tied to a plugin name at all.

comment:11 follow-up: helen5 months ago

I don't think that solves the deeper problem of there being a specific number that plugins and themes have to keep track of and use. Introducing a new body class just for targeting is exactly what I want to avoid, because no matter what you call it to get around the semantic part, it still sets a bad precedent and opens the door to people using it for, say, JS because they see it all the time but then we break it when we remove it later, because it's not supposed to be forever. The value should be defined by core, and plugins and themes using whatever method or methods would automatically get the right value for that version.

comment:12 in reply to: ↑ 11 johnbillion5 months ago

Replying to helen:

The value should be defined by core, and plugins and themes using whatever method or methods would automatically get the right value for that version.

The problem is that the toolbar height is needed in CSS, which has no means of providing a value that's reusable elsewhere, unlike PHP and JS.

If we don't want to keep the mp6 body class around, the best we can do is introduce a more semantic class name, such as mp6-admin-bar to go along with our current admin-bar.

comment:13 rachelbaker5 months ago

  • Cc rachel@… added

comment:14 lancewillett5 months ago

In 26100:

Twenty Fourteen: adjust fixed header top margin now that MP6 is merged. See #25906 props celloexpressions.

comment:15 follow-up: lancewillett5 months ago

Leaving this open so we can decide how to handle Twenty Fourteen installs in 3.7 and earlier. I'm sure this isn't the only theme that relies on the magic number of 28px, there's a good chance other themes will need to do the same thing for 3.8 and later.

comment:16 in reply to: ↑ 15 lancewillett5 months ago

Replying to lancewillett:

I'm sure this isn't the only theme that relies on the magic number of 28px, there's a good chance other themes will need to do the same thing for 3.8 and later.

@helen Case in point, see broken top of http://make.wordpress.org/core/. :P

comment:17 follow-up: helen5 months ago

I do know what the problem is. I'm trying to see if there's a way we can make it future proof for everybody. Maybe there isn't, but it's not that I don't understand what you're talking about.

This affects more than CSS, too. See gems like http://core.trac.wordpress.org/browser/trunk/src/wp-includes/js/admin-bar.js#L73. Why was it 32 before instead of 28? I guess so there was breathing room? Should we change it to 36 now? I don't know. Who knows. It's magic.

If nothing else, we need to document these with comments.

comment:18 lancewillett5 months ago

  • Priority changed from normal to low

comment:19 lancewillett5 months ago

@helen I've thought about this more, and still haven't come up with a better solution to the "magic number" than adding a class value that's unique to 3.8 and forward.

SergeyBiryukov5 months ago

comment:20 in reply to: ↑ 17 SergeyBiryukov5 months ago

Replying to helen:

I do know what the problem is. I'm trying to see if there's a way we can make it future proof for everybody.

We can probably just add a branch-x.x class to get_body_class() (like we do in the admin), and then use it to target older branches. See 25906.2.diff.

Last edited 5 months ago by SergeyBiryukov (previous) (diff)

comment:22 lancewillett5 months ago

Also in the Twenty Fourteen stylesheet above that rule in .2.diff we should add a comment for future reference, something like:

/* Legacy support for pre-3.8 toolbar height. */

SergeyBiryukov5 months ago

comment:23 SergeyBiryukov5 months ago

The issue with 25906.2.diff is that 3.6 and 3.7 won't have the new class, so we should add it in twentyfourteen_body_classes() too.

25906.3.diff also adds array_unique() to get_body_class() to make sure each class only appears once.

comment:24 lancewillett5 months ago

  • Priority changed from low to normal
  • Summary changed from Twenty Fourteen: Adjust Fixed Header Margin for MP6 Merge to Twenty Fourteen: adjust fixed header margin for MP6 merge

Talking with Nacin about this at WordCamp London, several options:

  1. Unfix the WP toolbar for Twenty Fourteen: only theme's header is fixed
  2. Unfix the Twenty Fourteen header (like we did in Twenty Thirteen, just remove that functionality)
  3. Remove the extra 4 pixels from the front-end WP toolbar -- I'd love @helen to chime in on that technique, could make the front vs. admin toolbars look different

My vote is 2.

comment:25 celloexpressions5 months ago

I think this is getting blown way out of proportion.

While this is a problem in general, for Twenty Fourteen only we're talking about a 4px gap, only for users who are logged in (or the rare cases when a site shows the admin bar to logged-out users), and only for older versions of WordPress. Considering that the vast majority of users will get Twenty Fourteen by updating to 3.8, I don't see much of a concern there, since this is a very minor bug (would be interesting to see stats on default themes installed on older versions of WordPress; after Twenty Thirteen didn't support it, do users expect any, let alone almost-complete compatibility?). And, the versions that we are theoretically supporting (back to 3.6) will have only been in the wild for a few months when 3.8 comes out; due to the shorter cycles I would expect a bigger concern with users that don't update and want to install Twenty Fourteen for 3.5 (which I believe we don't support) than 3.6 or 3.7.

So I'd like to add a fourth option:

  1. Leave a small gap below the admin bar and above the Twenty Fourteen fixed header for users who install Twenty Fourteen on older versions of WordPress.

For the other options, I'd say:

  1. Let's not set a precedent of doing this. And WP toolbar is more useful for logged-in users.
  2. Only for logged-in users/if the toolbar is showing, if we must. Please don't remove functionality because of a very minor back-compat concern for something that will probably rarely be used in an older WP environment. It's just a little 4px gap.
  3. Would solve the issue for Twenty Fourteen and everyone else, but doesn't really address the long-term problem (are we stuck with a 28px toolbar forever?). The mobile breakpoint where it jumps even bigger is more of a concern anyway, since themes need to add extra breakpoints to match WordPress'.

obenland5 months ago

celloexpressions5 months ago

Add min-height to the admin bar, to ensure that it always covers the minimum gap we have set.

comment:26 lancewillett5 months ago

  • Owner set to lancewillett
  • Resolution set to fixed
  • Status changed from new to closed

In 26569:

Twenty Fourteen: add min-height rule to the WP toolbar to ensure that no gap appears in pre-3.8 versions. Props celloexpressions, fixes #25906.

Note: See TracTickets for help on using tickets.