WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#11609 closed enhancement (fixed)

body_class and post_class bug with duplicate .page class possibility

Reported by: Frumph Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Themes Keywords: reporter-feedback
Focuses: Cc:

Description

the body_class and post_class are both able to return a .page class which causes problems with css elements and people will have to be a little smarter with body.page of course. However, having it as .post-<type> instead in the post-class will alleviate any of .page .page {} problems that will occur.

Diff included.

Attachments (2)

get_post_class.diff (510 bytes) - added by Frumph 4 years ago.
get_post_class diff, create the type as .post-<type>
11609.example.2.diff (421 bytes) - added by nacin 4 years ago.
Not really necessary to prevent a single collision (.page .page), but allows for more specific class names. Thinking wontfix though.

Download all attachments as: .zip

Change History (11)

Frumph4 years ago

get_post_class diff, create the type as .post-<type>

comment:1 nacin4 years ago

  • Component changed from General to Themes
  • Milestone changed from 2.9.1 to 3.0
  • Type changed from defect (bug) to enhancement

Bug -> enhancement: CSS allows for element specificity, as you mention. I see no harm in leaving body.post and div.post, and you're also not proposing to change that when post type is 'post' anyway.

2.9.1 -> 3.0: Point releases are reserved for security patches and blocker-level bugs.

This isn't backwards compatible. Any theme using div.page will need to change that to div.post-page.

I do like the idea. Why not, in addition to .post, we also add .post-type-{type}. See diff as what I mean.

nacin4 years ago

Not really necessary to prevent a single collision (.page .page), but allows for more specific class names. Thinking wontfix though.

comment:2 scribu4 years ago

  • Milestone 3.0 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Firstly, I don't see the problem with this CSS selector: body.page .page

Secondly, a class of .post-page is just confusing. Is it a post or is it a page?

Not to mention backwards incompatibility.

comment:3 Frumph4 years ago

Obviously you don't work with your own code.

If you have .page {} and it's referencing the post_class() then it will also reference the body_class which will make whatever you put in .page {} be over the entire site since it's a body class.

Then you will have to de-reference it in body.page {} what kind of backward mentality is that?

It's just bad code completely to keep it this way.

Not to mention with the new post types your going to have the double class there as well.

If you specify that its a POSTofthetype-<type>

Forget it, i'll just write a filter so it's done right like I do every other backward thing wordpress is doing.

comment:4 nacin4 years ago

  • Keywords 2nd-opinion added
  • Milestone set to 3.0
  • Resolution wontfix deleted
  • Status changed from closed to reopened

It looks like .page is the only duplicated class among body_class and post_class. I understand that makes it slightly more difficult for a theme developer to write CSS.

Defrecerencing? How difficult, really, is div.page? Or whatever other element you're using post_class on? Feel free to do whatever you'd like with a filter, but element specificity in CSS is hardly a hack. You should never rely on a class's uniqueness (especially one inserted by a CMS) -- that's why it's a class and not an id.

That said, with expanded custom post type support in 2.9 and 3.0, I think it would be a good idea to consider adding a post-type-{type} class, but not replacing the current {type} class. While it will duplicate the existing class, I can see how it can be helpful, given so many possibilities for overlap with custom post type names and other classes that may be used in a theme or by a widget. (Which, again, reminds us of why specificity is important.)

I'm okay with a wontfix, but I think we should re-open this and allow a core developer to view the second patch and make a determination first.

We have to be very careful in core not to cause backward compatability problems with themes and the various template tags. Very careful. (Plugins are a separate beast as they deal with hooks, APIs, and functionality, and are often developed by people with more of a programming background -- or any background at all -- than we see with themes.)

comment:5 hakre4 years ago

I have no problems with duplicate class names on different elements in the DOM. And even on the same element this won't be any harm I assume. Beware of the force of the C in CSS.

For strict themers out there I suggest to offer an additional parameter containing a prefix so that a themer can decide how certain class names should be prefixed. The default should have a small footprint, maybe the smallest one, '', an empty string.

Might be usefull for #9015.

comment:6 hakre4 years ago

  • Keywords 2nd-opinion removed

comment:7 hakre4 years ago

  • Keywords reporter-feedback added

comment:8 nacin4 years ago

I'm thinking .type-{$post->post_type} will be good to add. I think given the improved support for custom post types, the prefix can't hurt.

comment:9 automattor4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [13350]) Add .type-{post_type} to post_class. fixes #11609

Note: See TracTickets for help on using tickets.