Skip to content
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

Archive page updates and Post Format title support #137

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
4 participants
@lukemcdonald
Copy link

commented Jan 15, 2013

First off, my apologies for grouping all these into one pull request; I don't have much experience contributing to other repos. I didn't realize until now the commits were grouped under the cam pull request. In any case, all commits are related.

In short, I've added post format titles to the archive page via a function which defines the plural name for each supported post format. This same function is used to update the post format name in the wp_title filter. One last modification I did was swap the span tag in the archive table to wrap the heading descriptor text.

archive.php Outdated
} elseif ( is_tax( 'post_format' ) ) {
$format_labels = _s_post_format_labels();
printf( _x( '%s', 'post format label', '_s' ), $format_labels[get_post_format()] );

This comment has been minimized.

Copy link
@mfields

mfields Jan 18, 2013

Contributor

I do not think that the use of _x() is necessary here. We are merely pulling back the value of an array. No need to translate an dynamic value.

'quote' => __( 'Quotes', '_s' ),
'video' => __( 'Videos', '_s' ),
);

This comment has been minimized.

Copy link
@mfields

mfields Jan 18, 2013

Contributor

I think that it would be best to use _x() for these strings maybe with a context of "post-format-archive-label" or similar.

This comment has been minimized.

Copy link
@kovshenin

kovshenin Jan 18, 2013

Contributor

I agree with @mfields and I also think that the function should accept a key, and return the translated string already, rather than returning the whole array, i.e. _s_post_format_labels( 'quote' ); should retun Quotes.

This comment has been minimized.

Copy link
@mfields

mfields Jan 18, 2013

Contributor

Great idea about accepting a parameter! I agree.

This comment has been minimized.

Copy link
@lukemcdonald

lukemcdonald Jan 18, 2013

Author

Any thoughts on the function name? Since we will be returning a single string, I was thinking of changing the function name to one of the following:

  • _s_post_format_label() - singular
  • _s_get_post_format_label() - prefixed with get
  • _s_get_the_post_format_label() - similar to other function names e.g. get_the_date()

On a similar note, is there a preference on the term label vs. title (e.g. _s_get_the_post_format_title() )? It seems like title maybe a better name to stay more consistant with get_the_title().

As for the context name, should this be more generalized in case a user wants to use the label's outside of an archive page? Maybe just "post-format-label".

Let me know if I'm over thinking any of this :)

This comment has been minimized.

Copy link
@lukemcdonald

lukemcdonald Jan 18, 2013

Author

After thinking about it, maybe would could simplify the function name and pass a display value along with the key name:

_s_post_format_label( $format, $display = false ) {
    // return or print label code
}
@mfields

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2013

Hi Luke, I think that all of the commits in one pull request is fine. You can easily see a diff of all changes by pressing a little diff link at the bottom of the page. This looks pretty good to me. I've left a few notes inline. The only thing that looks a bit off is how the spans were moved in e1e78e7. This format seems like will will add confusion to translators + it will change already translated strings (this is a WordPress.com issue).

Update post format label function return the label and not an array o…
…f labels

* Changed _s_post_format_labels() to _s_post_format_label()
* Allow _s_post_format_label() to be passed a format and display option
* Added format label context to translated string
* Updated archive.php format title to use new format label function
* Updated wp_title filter in extras.php to use new format label function
@lukemcdonald

This comment has been minimized.

Copy link
Author

commented Jan 18, 2013

Hi @mfields, the span wraps were updated in d1036e7 as requested by @kovshenin in e1e78e7. I believe that should do the trick?

I also updated the functionality for retrieving the format labels in f86eb35. Instead of returning an array of labels, we now return a single translated string by passing a format key to _s_post_format_label().

else
$label = get_post_format_string( $format );

This comment has been minimized.

Copy link
@mfields

mfields Jan 19, 2013

Contributor

I'm not sure that using get_post_format_string() is a good fallback here. _s_post_format_label() is responsive for returning a plural form of the post format label while get_post_format_string() returns a singular representation. Mixing the two is a bit confusing imho. I think that the default array + the filter works great for the purposes here.

This comment has been minimized.

Copy link
@lukemcdonald

lukemcdonald Jan 19, 2013

Author

The reason (if I remember correctly) for get_post_format_string() fallback was so that if a child theme registers a new format that isn't included in the _s_post_format_label() $labels array, it would return the format name instead of nothing (undefined index). We could add the remaining formats to the array and cover all the basis.

$labels = array(
    'aside' => _x( 'Asides', 'post-format-archive-label', '_s' ),
    'link'  => _x( 'Links', 'post-format-archive-label', '_s' ),
    'image' => _x( 'Images', 'post-format-archive-label', '_s' ),
    'quote' => _x( 'Quotes', 'post-format-archive-label', '_s' ),
    'video' => _x( 'Videos', 'post-format-archive-label', '_s' ),

    'gallery' => _x( 'Galleries', 'post-format-archive-label', '_s' ),
    'status'  => _x( 'Statuses', 'post-format-archive-label', '_s' ),
    'audio'   => _x( 'Audio', 'post-format-archive-label', '_s' ),
    'chat'    => _x( 'Conversations', 'post-format-archive-label', '_s' ),
);

This comment has been minimized.

Copy link
@mfields

mfields Jan 19, 2013

Contributor

Child themes are a great reason to provide a fallback! I really like the idea of including all possible formats directly in the function. This way we can ensure that they are always plural.

This comment has been minimized.

Copy link
@lukemcdonald

lukemcdonald Jan 19, 2013

Author

Any arguments against the example provided? I wasn't sure about chat or status.

This comment has been minimized.

Copy link
@mfields

mfields Jan 19, 2013

Contributor

Status looks correct to me, although I really do not like it. "Audio" as a plural looks a bit odd as well. "Conversations" for chat should probably be changed because it will mismatch the label on the post screen in the admin. I'm not really sure what is best here though. I've pinged other members of the team for suggestions.

return $label;
else
print $label;

This comment has been minimized.

Copy link
@lukemcdonald

lukemcdonald Jan 22, 2013

Author

Need to swap print and return $label here.

@michaeldcain

This comment has been minimized.

Copy link
Member

commented Jun 26, 2013

Closing. Currently works as-is and adding plural versions of post format strings is a core ticket: http://core.trac.wordpress.org/ticket/23257

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.