Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 3 years ago

#29808 closed enhancement (fixed)

Post/paging navigation template tags

Reported by: obenland's profile obenland Owned by: johnbillion's profile johnbillion
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Themes Keywords: twenty-fifteen
Focuses: accessibility, template Cc:

Description

When default themes have to create their own functions to polyfil template tags, it's usually a good indicator that this is an opportunity for Core to make the lives of theme developers easier. An example for that would be the navigational template tags that have been in use since Twenty Ten.

Today, we not only see them in core bundled themes. Through forking default themes and building off of starter themes, a lot of themes in the Repository also use similarly structured template tags.

Post Navigation
Post navigation markup has not changed a lot since Twenty Ten. Eventually default themes wrapped it in its own function, but the output basically remained the same.

Paging Navigation
Like post navigation links to the previous/next post, the paging navigation links to the previous/next set of posts. With the exception of HTML5 support and a11y improvements, the tag has not changed since Twenty Ten.

It is something every theme has to implement one way or the other, which is why we have get_posts_nav_link() and posts_nav_link(). We should probably think about deprecating them.

Pagination Navigation
Out of the default themes, only Twenty Fourteen has used a pagination so far. Given the download numbers of plugins like WP Page-Navi, and our plugin-first approach however, a strong case can be made for the need of a pagination tag in core.

With the most recent improvements to paginate_links() (r28785, r29780), a pagination template tag would be a lot less involved than it had to be in Twenty Fourteen. It could basically just accept an arguments array that would be passed to paginate_links(), check if there are links, and then returns them wrapped in <nav> elements.

Attachments (14)

29808.diff (4.4 KB) - added by obenland 9 years ago.
29808.2.diff (9.0 KB) - added by obenland 9 years ago.
29808.3.diff (8.2 KB) - added by obenland 9 years ago.
Updated docs based on DrewAPicture's feedback.
29808.4.diff (7.6 KB) - added by obenland 9 years ago.
One more round of documentation improvements.
29808.5.diff (7.6 KB) - added by DrewAPicture 9 years ago.
29808.6.diff (7.6 KB) - added by johnbillion 9 years ago.
29808.7.diff (3.8 KB) - added by obenland 9 years ago.
29808.8.diff (5.1 KB) - added by obenland 9 years ago.
post navigation improvements
29808.9.diff (4.8 KB) - added by obenland 9 years ago.
Comments navigation template
29808.10.diff (7.5 KB) - added by obenland 9 years ago.
29808.11.diff (3.3 KB) - added by obenland 9 years ago.
Comments navigation template Pt.II
29808-pagination.diff (2.4 KB) - added by GaryJ 9 years ago.
Add Twenty Fourteen code to get_the_pagination()
29808.12.diff (4.6 KB) - added by obenland 9 years ago.
29808.13.diff (668 bytes) - added by obenland 9 years ago.
Allow list markup in post pagination.

Download all attachments as: .zip

Change History (98)

@obenland
9 years ago

#1 @obenland
9 years ago

I'd appreciate feedback on it, including naming, arguments, and documentation.

#2 follow-up: @paulwilde
9 years ago

I'd suggest removing the "navigation" class as it's a little too generic and in the majority of cases its more than likely to be used for a main navigation than it is for paging/pagination.

Last edited 9 years ago by paulwilde (previous) (diff)

#3 in reply to: ↑ 2 @obenland
9 years ago

Replying to paulwilde:

I'd suggest removing the "navigation" class as it's a little too generic and in the majority of cases its more than likely to be used for a main navigation than it is for paging/pagination.

True, though you could also say that it would be too generic for a main navigation. In the more recent default themes we used .navigation to have one class that we could use for styles that apply to all navigation elements throughout the site.

Additionally, theme developers could use it in combination with whatever class the specific template tag adds to that, so .paging.navigation for example.

#4 follow-up: @Frank Klein
9 years ago

I would recommend that we remove the HTML and the hardcoded arrows from the code. If theme developers want arrows, they could use an icon font and CSS.

#5 @obenland
9 years ago

  • Keywords twenty-fifteen added

Tagging with twenty-fifteen to keep track of it.

@obenland
9 years ago

#6 @obenland
9 years ago

Updated patch with more detailed function documentation.

This ticket was mentioned in IRC in #wordpress-dev by obenland. View the logs.


9 years ago

#8 @DrewAPicture
9 years ago

  • Keywords has-patch added

@obenland: Thanks for the more detailed docs in 29808.2.diff. Unfortunately, there's some work yet to do here to get it up to standard:

Overall:

  • In basically all of the parameter and hash notation descriptions there needs to be a description of what values it accepts, and what is the default, in that order. If it's markup and stuff, just describe it, if you want to use the markup, surround it with backticks so it doesn't get parsed as something else. The syntax we prefer is "Description. Accepts x, y, or z. Default abc."
  • The 'Optional.' string, when used, should fall before the description, instead of after

get_the_pagination():

  • s/Url/URL
  • Any example you list out should be in backticks to ensure they don't get parsed as something else
  • All of the descriptions in the hash notation should line up from top to bottom, so everything from $base to $add_args needs to move over to align with the two at the bottom
  • See the above point about 'Accepts' and 'Default' missing from the hash notations

the_pagination():

  • Good news! You don't need to rehash (heh) the hash notation here since you're just referencing arguments already defined. Just use an inline @see reference on the $args param line. Maybe something like " {@see get_the_pagination()} for available arguments".

_navigation_markup():

  • See above about 'Optional.' preceding the description. And we don't need to mark arguments as required, since everything is inferred to be required unless otherwise noted as optional.
  • 'Accepts' and 'Default' are missing from the parameter descriptions

@obenland
9 years ago

Updated docs based on DrewAPicture's feedback.

#9 @DrewAPicture
9 years ago

29808.3.diff looks much better, thanks. Couple more things :)

  • Missing the return descriptions for get_the_post_navigation(), get_the_paging_navigation(), get_the_pagination(), and _navigation_markup().

I think I missed these before, but anytime you have a template tag wrapping its corresponding get_ variation, you only need to document the defaults once (in the getter).

So just as you referenced the args defined in get_the_pagination() for the_pagination(), you would do the same thing for get_|the_post_navigation(), and get_|the_paging_navigation().

@obenland
9 years ago

One more round of documentation improvements.

@DrewAPicture
9 years ago

#10 @DrewAPicture
9 years ago

I'm satisfied with the docs in 29808.5.diff. Thanks @obenland!

@johnbillion
9 years ago

#11 @johnbillion
9 years ago

  • Keywords commit added; 2nd-opinion removed

As per a chat with obenland, we'll use the_posts_navigation() instead of the_paging_navigation() as the name better fits with core terminology.

This ticket was mentioned in Slack in #core by obenland. View the logs.


9 years ago

#13 @johnbillion
9 years ago

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

In 30065:

Introduce some new template functions for navigation:

  • get_the_post_navigation() and the_post_navigation() for navigation to the next and previous post.
  • get_the_posts_navigation() and the_posts_navigation()` for navigation to the next and previous page of posts.
  • get_the_pagination() and the_pagination() for paginated navigation between pages of posts. Uses paginate_links().

This reduces the need for themes to define their own sets of navigation functions.

Fixes #29808.
Props obenland.

#14 in reply to: ↑ 4 ; follow-up: @GaryJ
9 years ago

Replying to Frank Klein:

I would recommend that we remove the HTML and the hardcoded arrows from the code. If theme developers want arrows, they could use an icon font and CSS.

I'd agree with this, since adding RTL support still means jumping through hoops and supplying alternative strings for the default args, if you want the arrows (which are only visual indicators of a perceptual direction anyway) reversed for RTL.

How are the arrows read out in text-to-speech UAs, in terms of accessibility? Does it add anything that isn't already made clear from the words "previous" and "next"? If not, then that's another reason that the arrows should just be added via CSS, to match the theme styles, and not be part of the textual output.

get_the_pagination() doesn't have a meta-nav span around the arrows either, so it's inconsistent with the other functions, and doesn't provide a way to visually hide the arrows.

#15 in reply to: ↑ 14 @afercia
9 years ago

Replying to GaryJ:

How are the arrows read out in text-to-speech UAs, in terms of accessibility?

NVDA reads out them as:
&larr; "left arrow"
&rarr; "right arrow"

I'd recommend to reconsider the H1 as the heading of the navigation, should be H2 or maybe filterable.

#16 follow-up: @rianrietveld
9 years ago

Agree with aferica to reconsider the H1 as the heading of the navigation, should be H2 or maybe filterable.
If it's filterable, the H2 should be default.

Agree with removing &larr; &rarr; this could be far better done with CSS.

#17 follow-up: @rianrietveld
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The H1 issue is important enough to open the ticket to our opinion.

#18 in reply to: ↑ 17 ; follow-up: @obenland
9 years ago

  • Keywords commit removed

Replying to rianrietveld:

The H1 issue is important enough to open the ticket to our opinion.

What exactly is the issue here?

#19 in reply to: ↑ 18 @GaryJ
9 years ago

Replying to obenland:

Replying to rianrietveld:

The H1 issue is important enough to open the ticket to our opinion.

What exactly is the issue here?

Any pagination section title is not a top level heading for the content on that page. No browsers or UAs follow the heading algorithm given in HTML5 where a H1 should be used for each sectioning content.

#20 follow-up: @rianrietveld
9 years ago

Giving a h1 to the page navigation is way to much weight. An H1 should be given to the page title or archive title, not to a sub navigation, an H2 will be better. To give the theme developer flexibility a filter would be preferable, with an H2 as default.

Last edited 9 years ago by rianrietveld (previous) (diff)

#21 in reply to: ↑ 16 @GaryJ
9 years ago

Replying to rianrietveld:

Agree with removing &larr; &rarr; this could be far better done with CSS.

Interestingly, Twenty Fifteen already does this for older / newer posts pagination e.g.:

.pagination .next:before {
   content: "\f429";
   right: -1px;
}

#22 @GaryJ
9 years ago

The choice of class names is also not consistent. nav-links with nav-previous and nav-next inside (great), but then meta-nav inside those. If the arrows are to stay (hopefully not), can the class be namespaced with a nav- prefix too, i.e. nav-meta or nav-indicator?

#23 in reply to: ↑ 20 ; follow-up: @GrahamArmfield
9 years ago

Replying to rianrietveld:

Giving a h1 to the page navigation is way to much weight. An H1 should be given to the page title or archive title, not to a sub navigation, an H2 will be better. To give the theme developer flexibility a filter would be preferable, with an H2 as default.

I agree, the H1 heading level is definitely inappropriate as a default. H2 sounds like a sensible default and I think it would be great if it could be configurable by theme developers.

#24 in reply to: ↑ 23 @arush
9 years ago

Replying to GrahamArmfield:

Replying to rianrietveld:

Giving a h1 to the page navigation is way to much weight. An H1 should be given to the page title or archive title, not to a sub navigation, an H2 will be better. To give the theme developer flexibility a filter would be preferable, with an H2 as default.

I agree, the H1 heading level is definitely inappropriate as a default. H2 sounds like a sensible default and I think it would be great if it could be configurable by theme developers.

Adding my agreement. pagenav should not be a h1. Per WCAG there should not be more than one h1 on a page. In regard to the arrows, Jaws ignores them as if they are not there.

#25 @johnbillion
9 years ago

  • Keywords needs-patch added; has-patch removed

This ticket was mentioned in Slack in #accessibility by grahamarmfield. View the logs.


9 years ago

This ticket was mentioned in Slack in #accessibility by accessiblejoe. View the logs.


9 years ago

@obenland
9 years ago

#28 follow-ups: @obenland
9 years ago

  • Keywords has-patch added; needs-patch removed

Discussed this with arush, GrahamArmfield, joedolson, and acessibilityjoe during the contributor team meetup.

We agreed to remove the arrows and meta-nav items from all three functions, but leave the h1 tag.

While there is currently spotty support for HTML5 sectioning elements in screen readers, keeping it would foster the adoption of modern web standards. The most important part is that there is a screen reader text in the first place.

Last edited 9 years ago by obenland (previous) (diff)

#29 in reply to: ↑ 28 @GaryJ
9 years ago

  • Focuses accessibility added

Replying to obenland:

Discussed this with arush, GrahamArmfield, joedolson, and acessibilityjoe during the contributor team meetup.

We agreed to remove the arrows and meta-nav items from all three functions

Excellent, thank you.

but leave the h1 tag.

I find it hard to believe that all of those involved would have readily agreed to keeping the h1, but as no discussion was documented, I can't follow-up on the reasonings.

While there is currently spotty support for HTML5 sectioning elements in screen readers

The sectioning elements aren't the problem per se. The support of the outline algorithm is what's missing in all screen readers and graphical UAs. The HTML5 spec clearly recommends the use of heading hierarchy to convey document structure.

The fact you've highlighted the area is spotty, is surely sign that it's not ready for prime time Core usage?

keeping it would foster the adoption of modern web standards.

This is not the place to fight that battle. Not when the spec itself recognises that not a single implementation of this part of the spec currently exists and recommends not to follow it.

The most important part is that there is a screen reader text in the first place.

Agreed, but not if it's marked up in a way that is detrimental to screen reader users from understanding the document structure. This issue is now absolutely no different than r30072 / #30065, and to allow the h1 to stay would permit inconsistencies in the document structure.

Please, remove it, or at least filter it, so that if Twenty Fifteen and a different theme want to follow the completely-unsupported document outline for headings, they can do so when they want, without having to avoid using the functions as they stand.

#30 @rianrietveld
9 years ago

Strongly agree with GaryJ. We can't use his functionality now for a WCAG 2 accessible site where heading hierarchy is important.
If there was a filter to define the heading, every theme builder could fit this to his/her own theme needs.
Furthermore I'd like to refer to Steve Faulkners post on The HTML5 Document Outline

#31 in reply to: ↑ 28 @afercia
9 years ago

Replying to obenland:

We agreed to remove the arrows and meta-nav items from all three functions, but leave the h1 tag.

English is not my main language, but what I get from @accessiblejoe own words in the Slack archives you linked above, sounds different to me:

Multiple H1 per page are in the spec and referring back to Twenty Thirteen and other themes each post is an H1 because the spec allows it *but assistive tech still needs a hierarchy of heading levels*. Having one H1 per page with H2, H3 and so on gives assistive tech a means of navigation that users depend on.

and I couldn't agree more.

#32 @afercia
9 years ago

Bear with me, one more thing :-)
Just searched in /wp-includes/ for "h1" and there are a few hardcoded ones, mainly used when displaying errors (and that make sense) and just a couple in 'theme-compat'. If this passes, it would be the only h1 printed out in the content that theme authors wouldn't be able to change in any way.

#33 follow-ups: @joedolson
9 years ago

I have to apologize to Konstantin - I failed to realize that we were talking about a core tag, rather than a theme implementation when we discussed this. I still support leaving it as an H1 as a default, because it's impossible to know in any way what heading would be most appropriate. However, I do think that it should be filterable -- I don't think there should be any headings in the markup that aren't filterable by theme developers.

#34 in reply to: ↑ 33 @iamtakashi
9 years ago

Replying to joedolson:

However, I do think that it should be filterable.

I agree. It should definitely be filterable. As we see Twenty Fifteen development and Underscores, people have different opinions about heading structures with deferent focus and philosophy and theme developers will definitely need to filter it to meet their project brief and focus.

Last edited 9 years ago by iamtakashi (previous) (diff)

#35 in reply to: ↑ 33 @GaryJ
9 years ago

Replying to joedolson:

I still support leaving it as an H1 as a default, because it's impossible to know in any way what heading would be most appropriate.

Agreed that it's impossible to know for certain, but is h1 really the best default / most likely used for all WordPress themes that will be using these new functions? Even if there's no numbers available for what percentage of themes would use which tag, shouldn't we be catering for the best accessibility case (WCAG 2 as Rian mentioned) as the default, and let themes break off from that if they wish?

One doesn't have to know which is the right element, to know that it's not h1.

#36 follow-up: @joedolson
9 years ago

I don't see anywhere in WCAG 2.0 that specifies that the H1 can't cater to the best accessibility case. If you read http://www.w3.org/TR/WCAG20-TECHS/H42.html, it specifically states that "the layout could be presented with the first heading in each column at the same logical level (such as an h1)"; the important thing is whether items of an equivalent structural level are presented with equivalent logical levels.

Now, I would say that the way many themes have historically handled this is wrong, using an H1 for the page title, and when the blog title is an H1, I would say that this function should return an H2, because the navigation structure should be at a level below the main blog title. However, I also believe that the main blog title should not be a heading. If the blog title is not a heading, then the posts navigation is, in my opinion, a top-level structural piece of the page; along with the main content area and any other navigation or sidebars.

I think that we can debate around the question forever -- the fact is that the "ideal" heading hierarchy is not something that can be accomplished in a theme, because content and plug-ins will change it anyway. An "ideal" heading hierarchy is also subjective, and many different opinions can influence it.

To me, the important things here are only that 1) there is a heading and 2) that heading can be changed to suit the needs and requirements of the site using it.

#37 in reply to: ↑ 36 @GaryJ
9 years ago

Replying to joedolson:

If you read http://www.w3.org/TR/WCAG20-TECHS/H42.html, it specifically states that "the layout could be presented with the first heading in each column at the same logical level (such as an h1)";

if you care to finish that sentence, it carries on to say "or as found in the example, where the logical level reflects its importance in relation to the main content". It also says for the same example: "The content in the first and third columns is less important, and marked with h2". So, even putting any accessibility, document outline and SEO weightings aside, are you saying that the "Post Navigation" or similarly named sections are as important as the post content to which they are paginating through?

If the blog title is not a heading, then the posts navigation is, in my opinion, a top-level structural piece of the page; along with the main content area and any other navigation or sidebars.

I think our opinions are going to forever differ here then. The three-column example you referred to in WCAG 2 also differs with your opinion - the main column is h1, and the columns that contain Site Navigation and Related Links headings are each h2. WebAIM also disagrees with your opinion.

An "ideal" heading hierarchy is also subjective, and many different opinions can influence it.

I'm not trying to get "ideal" here. I'm trying to get the solution that I believe is the best combination of accessible, SEO beneficial, reflecting of importance related to other content on the page, and one that a greater proportion of developers will be happy to use as is, without wanting to filter it. I believe h2 is the best element for that.

To me, the important things here are only that 1) there is a heading and 2) that heading can be changed to suit the needs and requirements of the site using it.

On that we're agreed - I'd rather there were fewer people who felt the need to use that filter, than not, and that means catering for all reasonings of wanting a certain element, not just one.

#38 @joedolson
9 years ago

I don't see headings as reflecting importance, but structure; and navigation items, to me, are higher-level structural regions than content is. I think that's one of the key differences of opinions, here.

Even WebAIM's article includes qualifiers such as "generally", and allows for the presence of multiple H1 elements; the example I cited form WCAG was intended to illustrate that the author felt that either the usage illustrated in the example or a similar usage with H1's would be correct.

I don't feel any particular need to agree on this; but we do need to decide at what point we're going to simply move forward and accept that there is no ideal solution.

While I think that headings are an area that require deep thought when building a site, I don't see any particular value in expending a lot of effort in debating the question in the building blocks for a site. No matter what choice we make, it will not be the best option for all sites; and we don't fundamentally agree on which heading would be best for any site.

#39 follow-up: @joedolson
9 years ago

I'm going to come back with an alternate proposal here, because I walked away but couldn't stop considering it. I think that the fundamental problem here may be that WordPress does not generally specify any structural elements of themes from core that aren't self contained (such as navigation lists). Adding a heading into any context breaks that normal behavior, and is clearly leading to differences of opinion that aren't solvable, because ultimately no structure imposed by WordPress core is necessarily going to be a good fit for a given theme.

What if we remove the heading entirely, used aria-label to identify the nav element, and provided a filter by which developers could supply a heading if it was necessary for their use case.

#40 in reply to: ↑ 39 @GaryJ
9 years ago

Replying to joedolson:

What if we remove the heading entirely, used aria-label to identify the nav element, and provided a filter by which developers could supply a heading if it was necessary for their use case.

I'm +1 for that, assuming screen readers support that as well as reading a heading. I'm not sure how accurate it is, but http://www.w3.org/WAI/GL/wiki/Using_aria-label_to_provide_an_invisible_label has some info.

Assuming all is sufficiently well though, I agree it's a more workable solution than a heading that was intended to be visually hidden anyway. It also gets around the issue that #29699 covers too, where themes aren't guaranteed to have styles for .screen-reader-text.

#41 @joedolson
9 years ago

It does not have as good support as using a heading; older AT combinations won't support it, and it also won't show up in heading maps. However, as a brand-new function, this is only now going to begin to turn up, and so it would be a future-leaning choice, rather than depending on backwards compatibility (and the possibility that in the future we'd be having a debate about how we can remove the heading from the output when theme authors may have assumed it would be there.)

I'm in favor of adding new functions that use screen-reader-text as a class however; the more new features make use of that class, the more future potential there is for all themes to declare it.

Themes aren't generally going to change their functions of choice without also addressing the styles for that context of the theme.

#42 @afercia
9 years ago

Users first, some data from Webaim.org surveys:

January 2014

Predominant mechanism for finding page information
http://webaim.org/projects/screenreadersurvey5/#finding

Knowledge of and use of landmarks
http://webaim.org/projects/screenreadersurvey5/#landmarks

How many landmarks
http://webaim.org/projects/screenreadersurvey5/#numlandmarks
(this would need a ticket)

December 2010

Skip links:
http://webaim.org/projects/screenreadersurvey3/#skip

Heading Structure (and how many first level headings)
http://webaim.org/projects/screenreadersurvey3/#headings

Other surveys:
http://webaim.org/projects/

This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.


9 years ago

@obenland
9 years ago

post navigation improvements

This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.


9 years ago

#45 @obenland
9 years ago

29808.8.diff improves accessibility and templating logic:

  • Simplifies link texts.
  • Changes the order of post and posts navigation links.
  • Switches to use an h2 tag for screen reader text.

29808.9.diff introduces comments navigation template tags.
Not only can they be used in Twenty Fifteen (and could have been used in previous default themes), but they also provide parity with post navigation template tags.

@obenland
9 years ago

Comments navigation template

#46 follow-up: @rianrietveld
9 years ago

@obenland thanks for adding the H2, this way it's WCAG 2 accessible out-of-the-box.
As an addition: could you please consider using a filter for the heading. That way a theme developer could change the heading to fit her/his theme and the functionality is more flexible to use.

#47 in reply to: ↑ 46 @obenland
9 years ago

Replying to rianrietveld:

As an addition: could you please consider using a filter for the heading. That way a theme developer could change the heading to fit her/his theme and the functionality is more flexible to use.

It's not my decision to make.

We just spent the better part of this ticket discussing whether the text (which sole purpose is to provide context to screen readers!) should be an h1 or an h2 element. Now you want to give that all up and make it filterable? I don't understand.

I don't have strong feelings about whether it should be an h1 or an h2. Not anymore. But personally I feel like making it filterable would be over-engineering in all its glory. And go against every principle we have.

#48 follow-up: @joedolson
9 years ago

Given that this function doesn't do anything that can't be accomplished by other means - it's mostly a wrapper to simplify a common task - I think it's not crucial to filter it.

I do note that it's standard for template functions to provide a means to alter their output html, so I'm not sure why is such a big deal to offer that in this function.

#49 @afercia
9 years ago

Also, maybe I'm missing something, how to change the heading text? I mean the text passed to the private function _navigation_markup().
Say my posts are custom post types: "Products", "Reviews", whatever, my heading will always be "Post navigation" while I would like to have a more appropriate text: "Products navigation" etc.

Anyway, in get_the_post_navigation() the passed text should be "Posts navigation" (plural).

This ticket was mentioned in Slack in #accessibility by michael. View the logs.


9 years ago

#51 @michaelbeil
9 years ago

A h2 as the default heading and a filter for everyone else who needs a different heading seems to be the perfect flexible solution so it's possible to manipulate the output.

Last edited 9 years ago by michaelbeil (previous) (diff)

This ticket was mentioned in Slack in #core by obenland. View the logs.


9 years ago

#53 in reply to: ↑ 48 @afercia
9 years ago

Replying to joedolson:

I do note that it's standard for template functions to provide a means to alter their output html, so I'm not sure why is such a big deal to offer that in this function.

For example, the new the_archive_title() does provide a $before and $after arguments to be used before and after title. That's quite standard.

This ticket was mentioned in Slack in #core by obenland. View the logs.


9 years ago

#55 @GaryJ
9 years ago

As raised by @nathanrice - the wrapping elements (e.g. <div class="nav-previous">...</div> et al) don't appear to be filterable, or be accepted as arguments to the functions, meaning that themes with an existing structure can't use these functions without breaking CSS class hooks.

Could the wrapping elements be changeable via parsing args, with the defaults staying as what is currently committed?

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


9 years ago

@obenland
9 years ago

#57 @johnbillion
9 years ago

In 30457:

Improvements to the output of the new post navigation template functions, including swapping the position of the previous and next links.

See #29808
Props obenland

@obenland
9 years ago

Comments navigation template Pt.II

This ticket was mentioned in Slack in #core-themes by iamtakashi. View the logs.


9 years ago

#59 @iandstewart
9 years ago

It'd be great to see custom comments navigation functions in one place, core, and out of Twenty Fifteen (and other themes).

I believe the template-tags.php naming that I think we've inherited from _s dev was first suggested by @matveb for _s to inspire us, or more accurately shame us, into one day getting everything out of there and into core. Themes shouldn't have custom template tags.

This ticket was mentioned in Slack in #core by obenland. View the logs.


9 years ago

#61 @obenland
9 years ago

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

I've opened #30589 for the comment navigation template tags in a future release.

#62 follow-up: @GaryJ
9 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Now I've had chance to implement the new template tags, I think we have problems with get_the_pagination().

  • There's no total arg passed through to paginate_links() so it always returns null. This alone makes it fail.
  • There's no current arg pass through to paginate_links(), so even if there's a total, the function has to assume we're on the first page, so fails.

Both of these presumably would have been caught by unit tests had there been any.

Something like the following seems to fix it though I've not tested extensively.

'total'   => $GLOBALS['wp_query']->max_num_pages,
'current' => get_query_var( 'paged' ) ? absint( get_query_var( 'paged' ) ) : 1,

Furthermore, the get_the_pagination() function has a non-user-definable arg for type that is plain. The associated comment reads: "// Make sure we get plain links, so we can work with it.". I'm assuming this means that the links are to be returned as a string rather than array? Some folks will semantically want their list of page numbers to be returned as a set of list item elements (type set to list, which still returns as a string). Can this arg be moved inside wp_parse_args() so the developer can override it if they want the more convoluted markup?

Last edited 9 years ago by GaryJ (previous) (diff)

This ticket was mentioned in Slack in #core by garyj. View the logs.


9 years ago

#64 @GaryJ
9 years ago

Also, the Previous and Next links on numerical pagination don't work, at least with permalinks. Twenty Fourteen does all sorts of things with base, format and query strings. Presumably this would still be needed too?

@GaryJ
9 years ago

Add Twenty Fourteen code to get_the_pagination()

#65 @GaryJ
9 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

Patch added, which incorporates most of the Twenty Fourteen pagination code into get_the_pagination().

This ticket was mentioned in Slack in #core by garyj. View the logs.


9 years ago

#67 in reply to: ↑ 62 ; follow-up: @obenland
9 years ago

Replying to GaryJ:

Now I've had chance to implement the new template tags, I think we have problems with get_the_pagination().

Can you tell us where exactly you experienced the bug with steps to reproduce?

#68 in reply to: ↑ 67 ; follow-up: @GaryJ
9 years ago

Replying to obenland:

Replying to GaryJ:

Now I've had chance to implement the new template tags, I think we have problems with get_the_pagination().

Can you tell us where exactly you experienced the bug with steps to reproduce?

If was on a Genesis Framework theme, but amounted to little more than calling the_pagination() on the genesis_after_endwhile hook.

All other calls to paginate_links() that I could find in WP core all pass in total and current. Looking at the code for that function, the args are merged with the defaults, and if those values persist, then the guard clause for total returns early, and the current page is set as 1.

#69 in reply to: ↑ 68 ; follow-up: @obenland
9 years ago

Replying to GaryJ:

If was on a Genesis Framework theme, but amounted to little more than calling the_pagination() on the genesis_after_endwhile hook.

Is there a way to reproduce the error with a default theme?

Within paginate_links(), total and current are set based on the current query. If there is no current query, you'd have to pass those values to the_pagination() manually, yes.

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


9 years ago

#71 in reply to: ↑ 69 ; follow-up: @GaryJ
9 years ago

  • Keywords has-patch removed

Replying to obenland:

Replying to GaryJ:

If was on a Genesis Framework theme, but amounted to little more than calling the_pagination() on the genesis_after_endwhile hook.

Is there a way to reproduce the error with a default theme?

Within paginate_links(), total and current are set based on the current query. If there is no current query, you'd have to pass those values to the_pagination() manually, yes.

My bad. Found the discrepancy, and my observations re total and current are invalid.

Why does the $args['type'] need to be hard-coded as plain?

@obenland
9 years ago

#72 @obenland
9 years ago

In 29808.12.diff:

Renames pagination function to make it fit in with the post/posts of existing link template tags.
post denominates a single post, posts a set of posts.
This also changes paging to posts, to be more concise in naming and support that differentiation.

#73 in reply to: ↑ 71 ; follow-up: @obenland
9 years ago

  • Keywords dev-feedback removed

Replying to GaryJ:

Why does the $args['type'] need to be hard-coded as plain?

It's just important that it's not an array, obviously. We could change that in 4.2.

This ticket was mentioned in Slack in #core by obenland. View the logs.


9 years ago

#75 @johnbillion
9 years ago

  • Keywords commit added

I'm happy with the function and class name changes in 29808.12.diff.

#76 in reply to: ↑ 73 @GaryJ
9 years ago

Replying to obenland:

Replying to GaryJ:

Why does the $args['type'] need to be hard-coded as plain?

It's just important that it's not an array, obviously. We could change that in 4.2.

Why could it not be changed now? There's still a commit to go in, that renames functions; removing a hard-coded arg so that it uses the default of the same (overridable) value is a relatively minor change.

I'm not asking it to change from plain - but just give theme developers the flexibility to output with or without list markup (that might be needed for styling or semantic purposes), without us having decided for them. Twenty Fifteen may not use list item markup, but I see no reason to defer it to 4.2 when there's still a related commit to go in.

@obenland
9 years ago

Allow list markup in post pagination.

#77 @johnbillion
9 years ago

In 30819:

Allow the type argument to be passed through get_the_pagination() as long as its value isn't array.

See #29808

#78 @johnbillion
9 years ago

In 30820:

Allow the type argument to be passed through get_the_pagination() as long as its value isn't array.

For trunk.

See #29808

#79 @johnbillion
9 years ago

In 30823:

Rename (get_)the_pagination() to (get_)the_posts_pagination() for clarity.

See #29808
Props obenland

#80 @johnbillion
9 years ago

In 30824:

Switch to (get_)the_posts_pagination() in Twenty Fifteen.

See #29808
Props obenland

#81 @johnbillion
9 years ago

In 30825:

Rename (get_)the_pagination() to (get_)the_posts_pagination() for clarity.

Merges [30823] to the 4.1 branch

See #29808
Props obenland

#82 @johnbillion
9 years ago

In 30826:

Switch to (get_)the_posts_pagination() in Twenty Fifteen.

Merges [30824] to the 4.1 branch.

See #29808
Props obenland

This ticket was mentioned in Slack in #core by garyj. View the logs.


9 years ago

#84 @johnbillion
9 years ago

  • Keywords commit removed
  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.