WordPress.org

Make WordPress Core

Opened 13 months ago

Last modified 2 months ago

#38545 new enhancement

Add span element to get_the_archive_title()

Reported by: Kaira Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.6.1
Component: Taxonomy Keywords: has-patch needs-refresh
Focuses: Cc:

Description

As a theme developer (@Kaira) I'd like to request this small feature of adding a <span> element to get_the_archive_title() function.

This is to allow theme developers to target and style the archive title with CSS to be able to add styling to it.

I've experienced the need for this when developing themes for WordPress.org and for when I'm building client websites.

Thanks

Attachments (11)

ADDED FEATURE REQUEST - Example - get_the_archive_title.php (2.9 KB) - added by Kaira 13 months ago.
This is get_the_archive_title() with my added feature request
get_the_archive_title().php (2.9 KB) - added by Kaira 13 months ago.
The original get_the_archive_title() in /wp-includes/general-template.php
01.png (779.9 KB) - added by Kaira 12 months ago.
02.png (168.6 KB) - added by Kaira 12 months ago.
38545.diff (2.8 KB) - added by milindmore22 10 months ago.
proper patch
38545.patch (22.3 KB) - added by shireling 10 months ago.
38545.2.patch (4.7 KB) - added by shireling 10 months ago.
single span, with user defined class name
38545.3.patch (4.7 KB) - added by shireling 10 months ago.
Previous patch was inverted - this replaces 38545.2.patch
38545.4.patch (5.2 KB) - added by shireling 10 months ago.
Updated inline docs, updated translator notes, escaped new class name
38545.5.patch (4.5 KB) - added by grapplerulrich 2 months ago.
38545.6.patch (4.6 KB) - added by grapplerulrich 2 months ago.

Download all attachments as: .zip

Change History (41)

@Kaira
13 months ago

This is get_the_archive_title() with my added feature request

@Kaira
13 months ago

The original get_the_archive_title() in /wp-includes/general-template.php

#1 @jcastaneda
12 months ago

Is that not what the_archive_title() does already?

The function accepts two arguments, the first one being what will be printed before the title and the second what will be printed after.

The only one that adds any extra markup would be the author.

@Kaira
12 months ago

@Kaira
12 months ago

#2 @Kaira
12 months ago

Hi @jcastaneda

Thanks for the response... I've uploaded 2 images to explain more on what I mean.

Thanks

#3 @swissspidy
10 months ago

#39646 was marked as a duplicate.

#4 @SergeyBiryukov
10 months ago

  • Keywords needs-patch added

ADDED FEATURE REQUEST - Example - get_the_archive_title.php should be converted to a proper patch, see https://make.wordpress.org/core/handbook/tutorials/trac/submitting-a-patch/.

@milindmore22
10 months ago

proper patch

#5 @milindmore22
10 months ago

  • Keywords has-patch added; needs-patch removed

#6 @Kaira
10 months ago

Hi @sergeyBiryukov / @milindmore22 .. does this mean the feature request has been submitted?

Last edited 10 months ago by Kaira (previous) (diff)

#7 @milindmore22
10 months ago

Hello @Kaira,
I have attached a proper patch for you request, soon someone from WordPress core team will review the patch and merge it in future release let's wait for them to review and hope for the best,

Regards,
Milind

#8 @Kaira
10 months ago

Hi @milindmore22

Thanks very much... hoping they do :)

Thanks

@shireling
10 months ago

#9 @shireling
10 months ago

Do we want a span on both halves of the title? That might make it easier for users/themers to target the desired section.

It looks like an extra xml file snuck into the patch I just uploaded, but this diff should be better:

https://patch-diff.githubusercontent.com/raw/chad1008/wordpress-develop/pull/1.diff

#10 @theMikeD
10 months ago

I think the first patch is a better solution although I don't understand why Author was ignored.

In @shireling patch I would suggest against the second span (the one around the content, what you call archive-title). The single span should be enough of a target. Or change it to

$format ='<span class="archive-label"><span>%s</span>%s</span>';

The other way to handle this would be to expand the filter to take (using @shireling patch for the var names) $title, $format, $type, $title_classes, $name or something along those lines. This way theme authors could wrap them as they see fit. This also dodges the what-would-certainly-be-an-endless discussion on what the class name should be called.

As another observation, the : is problematic for me. Perhaps we can get rid of that altogether and add a CSS rule to add it back in? This way authors can determine if they want a : or - or whatever.

#11 @milindmore22
10 months ago

The author was already in the span that's why I ignored it.

<span class="vcard">' . get_the_author() . '</span>

#12 @Kaira
10 months ago

I would agree to having the span on only the label... For example I've had a few times where users want to hide the word "Category:"... and with the one span on the label, that is enough to style the label different to the name/title with CSS.

#13 @shireling
10 months ago

Cool, if it's going to be on the label I think the single span solution can work nicely. I'm uploading a new patch - this one applies a single span to just the label (first) half of the title. The vcard title on author archives title remains in place.

It also modifies the_archive_title(); and get_the_archive_title(); to accept an argument defining the class name - if left blank, you get a span without any class assigned.

@shireling
10 months ago

single span, with user defined class name

@shireling
10 months ago

Previous patch was inverted - this replaces 38545.2.patch

#14 @Kaira
10 months ago

Great... thanks :)

#15 @swissspidy
10 months ago

  • Keywords needs-refresh added

38545.3.patch.

  • needs to update the inline docs for the function
  • the class names should be escaped
  • this is not good for i18n. For example, right now we have __( 'Tag: %s' ), suggested ist: __( 'Tag: ' ). And the translator comment isn't adjusted.

See also #31237.

#16 @Kaira
10 months ago

Hey @shireling

Can you help with this?
Or how can I make the changes?

Sorry I'm not too clued up on submitting changes :D

@shireling
10 months ago

Updated inline docs, updated translator notes, escaped new class name

#17 @shireling
10 months ago

More than happy to try, thanks for looking that over @swissspidy :)

I wasn't sure if it would be best practice to remove the entire translator note on strings where there were no variables, or just the reference to the variable. For now (to use @swissspidy's example)

/* translators: Tag archive title. 1: Tag name */
has been changed to
/* translators: Tag archive title. */

Please let me know if it would be better to remove the entire note on the lines that have no variables in them.

@Kaira - I'm new to submitting changes as well - this has been my bible :)

Last edited 10 months ago by shireling (previous) (diff)

#18 @Kaira
10 months ago

@shireling Thanks for the link... I'll go through that.

#19 @melchoyce
5 months ago

@swissspidy How do you think this is looking now? Ready for merge?

#20 follow-up: @swissspidy
5 months ago

Things like __( 'Archives: ' ); aren't really great fro translators though. Then I'd rather prefer <span> elements inside the strings.

@SergeyBiryukov @ocean90 What do you guys think about that?

#21 in reply to: ↑ 20 @SergeyBiryukov
5 months ago

Replying to swissspidy:

Things like __( 'Archives: ' ); aren't really great fro translators though. Then I'd rather prefer <span> elements inside the strings.

The trailing space should be moved out of the strings, otherwise I think it's fine. Maybe @ocean90 can take a look if any locales do something with these strings that would no longer be possible with the patch?

#22 follow-up: @melchoyce
3 months ago

@ocean90 Would you have time to take a look at this during 4.9?

#23 in reply to: ↑ 22 @shireling
3 months ago

I'm happy to put together another patch moving those spaces out of the strings, but I'll hold off to see if any other changes are needed as well :)

#24 @javorszky
2 months ago

Hello,

Great idea for improving it! :)

Few observations:

if span_class is empty, the output will have a trailing space before the closing > of the opening <span>. So it's be <span >Category:</span> foo. That could be solved by adding the space here (notice the space before "class"):

if ( ! '' == $span_class ) { 
	$span_class = sprintf( ' class="%s"', sanitize_html_class( $span_class ) ); 
}

This currently doesn't work with RTL languages. For that to work, you'd have to do

/* translators: Archive title format. 1: span classes, 2: archive type name, 3: archive term */
$format = _x( '<span%1$s>%2$s</span>%3$s', 'archive title format' );`
$title = sprintf( $format, $span_class, $type, $name );

That way any RTL language could have a translation that would essentially be

$format = '%3$s<span%1$s>%2$s</span>

after translation and the sprintf would still work.

From a technical point of view, if you accept span class as an array, or break it up by spaces, then you can add the author's vcard classname to the array. So it'd be

if ( ! '' == $span_class && is_string( $span_class ) ) { 
	$span_class = explode( ' ', $span_class );
	$span_class = sprintf( 'class="%s"', sanitize_html_class( $span_class ) ); 
}

// then later
} elseif ( is_author() ) { 
    /* translators: Author archive title. %s: Author name */ 
    $type = __( 'Author: ' ); 
    $span_class[] = 'vcard';
    $name = get_the_author(); 
} elseif ( is_year() ) {

// and finally

$title = sprintf( $format,' classes="' . implode( ' ', $span_class ) . '"', $type, $name ); 

And that's about it from me for now. Let me know what you think.

#25 follow-up: @BinaryMoon
2 months ago

Since I am keen to see this added in 4.9 I asked on Twitter if anyone could review it and @glueckpress replied with:

On my phone til next week, so replying here. 2 things come to mind:
1/ string should not end with space.
2/ Using _x() for context could be useful to make string unique for localisers.

The thread is here:
https://twitter.com/glueckpress/status/906235746549420032

Thanks also to @javorsky for the feedback.

Hope this helps 😀

Last edited 2 months ago by BinaryMoon (previous) (diff)

#26 in reply to: ↑ 25 @javorszky
2 months ago

You're welcome :)

P.S.: There's a "z" in my name too ;)

Replying to BinaryMoon:

Since I am keen to see this added in 4.9 I asked on Twitter if anyone could review it and @glueckpress replied with:

On my phone til next week, so replying here. 2 things come to mind:
1/ string should not end with space.
2/ Using _x() for context could be useful to make string unique for localisers.

The thread is here:
https://twitter.com/glueckpress/status/906235746549420032

Thanks also to @javorsky for the feedback.

Hope this helps 😀

#27 follow-up: @grapplerulrich
2 months ago

Here are some of the changes that I made:

  • changes with the spacing
  • fixed a few alignment issues
  • improved part of the documentation
  • Replaced the large if statement with a smaller one

@javorszky The order does not need to be able to be changed for RTL. That is automatically done by the browser. Though there maybe some other languages where that may be necessary but I am not sure which ones.

#28 in reply to: ↑ 27 @javorszky
2 months ago

Bear in mind that some LTR languages might have different word orders. English language has a hierarchy of thing: member things, ie "Category: cats"

Hungarian on the other hand (my mothertongue) would potentially display the same as "'macskák' kategória", which translated would be "(the) cats category".

Control over word / concept order is important.

#29 @grapplerulrich
2 months ago

@javorszky Yea, Japanese do that to sometimes but it seems like the translations that I checked did not change the order of the term and taxonomy. https://translate.wordpress.org/projects/wp/dev/hu/default?filters%5Bstatus%5D=either&filters%5Boriginal_id%5D=202084&filters%5Btranslation_id%5D=9661611

#30 @javorszky
2 months ago

@grapplerulrich the fact that one language did not do it in this case is not grounds for not including the ability to change word order. Remember, with translations, you're preparing the code so it can be translated into ANY of the languages, even ones you don't even know about.

I'd highly recommend you include numbered placeholders so the order can be moved around.

Note: See TracTickets for help on using tickets.