New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conflicts with hybrid_body_class and schema plugin that plays w/ body_class #17

Closed
krogsgard opened this Issue Jan 17, 2013 · 11 comments

Comments

Projects
None yet
5 participants
@krogsgard

krogsgard commented Jan 17, 2013

Hey Justin,

I've got a problem where a plugin that's doing some stuff with the body_class function to insert an itemprop parameter for schema data is not playing nicely with the hybrid_body_class() function. I talked to Andrew Norcross about it (the plugin dev) and he asked why hybrid_body_class couldn't use the regular body_class filter to add classes, instead of the join that it's using now. Any reason for that?

Here are the appropriate links:

Schema plugin body_class function: http://plugins.svn.wordpress.org/schema-creator/tags/1.042/schema-creator.php ~ line 425

Hybrid context.php https://github.com/justintadlock/hybrid-core/blob/master/functions/context.php

broken body class declaration screenshot: http://cl.ly/image/3k3A2M1k1t1T

Tract ticket that discusses why Andrew used the function he did: http://core.trac.wordpress.org/ticket/21766

Thoughts? The hybrid_body_class() function, in combination w/ the schema generation plugin, tore apart one of my dev sites today. If I could get them to play nicely together, it'd be great. In the meantime, I've disabled the setting to add the schema data to the body tag in the plugin settings.

@justintadlock

This comment has been minimized.

Show comment
Hide comment
@justintadlock

justintadlock Jan 17, 2013

Owner

The plugin is using the body_class hook incorrectly. The WP function get_body_class() returns and the body_class hook expects an array of classes and classes only, not additional HTML. Granted, Hybrid Core is doing something a little non-standard. But, even if I pulled get_body_class() in directly (a possibility) instead of just using the body_class hook, it'd still break.

I'm open to suggestions for making hybrid_body_class() better, but it must provide backwards compatibility for the hybrid_body_class hook, which has been around since 2008 (before body_class() ever existed) and is used by many people.

But, this is something that really needs to be addressed in core.

Owner

justintadlock commented Jan 17, 2013

The plugin is using the body_class hook incorrectly. The WP function get_body_class() returns and the body_class hook expects an array of classes and classes only, not additional HTML. Granted, Hybrid Core is doing something a little non-standard. But, even if I pulled get_body_class() in directly (a possibility) instead of just using the body_class hook, it'd still break.

I'm open to suggestions for making hybrid_body_class() better, but it must provide backwards compatibility for the hybrid_body_class hook, which has been around since 2008 (before body_class() ever existed) and is used by many people.

But, this is something that really needs to be addressed in core.

@justintadlock

This comment has been minimized.

Show comment
Hide comment
@justintadlock

justintadlock Jan 17, 2013

Owner

Also, you can always use the <?php body_class(); ?> in your themes instead of class="<?php hybrid_body_class(); ?>".

Owner

justintadlock commented Jan 17, 2013

Also, you can always use the <?php body_class(); ?> in your themes instead of class="<?php hybrid_body_class(); ?>".

@norcross

This comment has been minimized.

Show comment
Hide comment
@norcross

norcross Jan 17, 2013

I agree the function in my plugin isn't the intended use (you can see my ticket for it, and @markjaquith's response with the function I'm using now). As it stands, the body_class function doesn't allow for anything other than classes, which prevents my ability to add the schema markup.

Also, based on datestamps, I assumed that your function predated the core version of body_class. Lemme play around with it to see if a workable solution can be found, since currently there is no other way to add the itemprop and itemscope into the body tag.

norcross commented Jan 17, 2013

I agree the function in my plugin isn't the intended use (you can see my ticket for it, and @markjaquith's response with the function I'm using now). As it stands, the body_class function doesn't allow for anything other than classes, which prevents my ability to add the schema markup.

Also, based on datestamps, I assumed that your function predated the core version of body_class. Lemme play around with it to see if a workable solution can be found, since currently there is no other way to add the itemprop and itemscope into the body tag.

@justintadlock

This comment has been minimized.

Show comment
Hide comment
@justintadlock

justintadlock Jan 17, 2013

Owner

What I propose is that we push for a new core function called body_attributes() as well as a function called get_body_attributes() with an accompanying hook. Here's a basic look at how it could be done:

function get_body_attributes() {

    $attributes = array(
        'class' => join( ' ', get_body_class() ),
    );

    return apply_filters( 'body_attributes', $attributes );
}

function body_attributes() {

    $out = '';

    foreach ( get_body_attributes() as $name => $value )
        $out .= " {$name}="{$value}"; // sanitize

    echo $out;
}

With the theme review team, we can very easily make this a standard as part of the guidelines. We just need to get it added to core first.

Owner

justintadlock commented Jan 17, 2013

What I propose is that we push for a new core function called body_attributes() as well as a function called get_body_attributes() with an accompanying hook. Here's a basic look at how it could be done:

function get_body_attributes() {

    $attributes = array(
        'class' => join( ' ', get_body_class() ),
    );

    return apply_filters( 'body_attributes', $attributes );
}

function body_attributes() {

    $out = '';

    foreach ( get_body_attributes() as $name => $value )
        $out .= " {$name}="{$value}"; // sanitize

    echo $out;
}

With the theme review team, we can very easily make this a standard as part of the guidelines. We just need to get it added to core first.

@norcross

This comment has been minimized.

Show comment
Hide comment
@norcross

norcross Jan 17, 2013

that's exactly what I put in that ticket (http://core.trac.wordpress.org/ticket/21766) but I didn't get much traction. Feel free to revive that one.

norcross commented Jan 17, 2013

that's exactly what I put in that ticket (http://core.trac.wordpress.org/ticket/21766) but I didn't get much traction. Feel free to revive that one.

@developdaly

This comment has been minimized.

Show comment
Hide comment
@developdaly

developdaly Jan 18, 2013

Why not a body() function that can be filtered with body_class() and body_attributes() -- maintaining body_class() functionality and extending it to account for any new scenario?

<<?php body( array( 'classes' => array( 'my_class', 'your_class' ), 'attributes' => array( 'id' => 'my-id' ) ); ?>>

developdaly commented Jan 18, 2013

Why not a body() function that can be filtered with body_class() and body_attributes() -- maintaining body_class() functionality and extending it to account for any new scenario?

<<?php body( array( 'classes' => array( 'my_class', 'your_class' ), 'attributes' => array( 'id' => 'my-id' ) ); ?>>

@ryanve

This comment has been minimized.

Show comment
Hide comment
@ryanve

ryanve Jan 19, 2013

It'd be way more useful to have generic function for formatting attributes rather than one specific only to the body tag. It's better to make functions that we can use lots of times rather than ones that only get one use. Consider also that some people may use body_class() on the <html> element. Themes can use filters on the tags:

<body <?php echo apply_atomic('body_attrs', ''); ?>>
<?php echo apply_atomic('body_tag', '<body>'); ?>

The tag() and attrs() and parseAttrs() in phat are really solid if you want to use those. I can write WP-specific versions of those with filters for hybrid-core if you want me too. What I'd really like to see is have them in WP.

ryanve commented Jan 19, 2013

It'd be way more useful to have generic function for formatting attributes rather than one specific only to the body tag. It's better to make functions that we can use lots of times rather than ones that only get one use. Consider also that some people may use body_class() on the <html> element. Themes can use filters on the tags:

<body <?php echo apply_atomic('body_attrs', ''); ?>>
<?php echo apply_atomic('body_tag', '<body>'); ?>

The tag() and attrs() and parseAttrs() in phat are really solid if you want to use those. I can write WP-specific versions of those with filters for hybrid-core if you want me too. What I'd really like to see is have them in WP.

@justintadlock

This comment has been minimized.

Show comment
Hide comment
@justintadlock

justintadlock May 15, 2013

Owner

@ryanve I'm not sure how I missed your post. I've already added some stuff to handle this, but I'm really digging your idea and will probably revisit this soon.

Owner

justintadlock commented May 15, 2013

@ryanve I'm not sure how I missed your post. I've already added some stuff to handle this, but I'm really digging your idea and will probably revisit this soon.

@ryanve

This comment has been minimized.

Show comment
Hide comment
@ryanve

ryanve May 16, 2013

@justintadlock Cool. I just fixed the broken link in my comment above and updated both related trac tickets (23236 and 23237). I put a wphat.php file in the phat repo where I'm working on it adapting it to WordPress.

ryanve commented May 16, 2013

@justintadlock Cool. I just fixed the broken link in my comment above and updated both related trac tickets (23236 and 23237). I put a wphat.php file in the phat repo where I'm working on it adapting it to WordPress.

@justintadlock

This comment has been minimized.

Show comment
Hide comment
@justintadlock

justintadlock May 16, 2013

Owner

There is now the new {$prefix}_body_attributes hook that Hybrid Core offers. See: 144c3bb

That's the best I can really offer at the moment. I'll be more than happy to help with any plugin integration for using this hook. For now, I'm going to consider this issue closed as far as Hybrid Core is concerned.

Owner

justintadlock commented May 16, 2013

There is now the new {$prefix}_body_attributes hook that Hybrid Core offers. See: 144c3bb

That's the best I can really offer at the moment. I'll be more than happy to help with any plugin integration for using this hook. For now, I'm going to consider this issue closed as far as Hybrid Core is concerned.

@justintadlock

This comment has been minimized.

Show comment
Hide comment
@justintadlock

justintadlock May 16, 2013

Owner

@ryanve Thanks for throwing that together. I'll take a look through it.

Owner

justintadlock commented May 16, 2013

@ryanve Thanks for throwing that together. I'll take a look through it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment