Automattic / _s

Hi. I'm a starter theme called _s, or underscores, if you like. I'm a theme meant for hacking so don't use me as a Parent Theme. Instead try turning me into the next, most awesome, WordPress theme out there. That's what I'm here for.
http://underscores.me/
GNU General Public License v2.0
10.96k stars 3.12k forks source link

wp_link_pages should not be inside entry-content #1294

Open dshanske opened 6 years ago

dshanske commented 6 years ago

wp_link_pages should not be inside entry-content, as this is not content.

grappler commented 6 years ago

This has been so since the initial commit.

@dshanske Where would you place wp_link_pages()?

dshanske commented 6 years ago

In the entry footer function is my suggestion. It is not content, it is navigation. I know it has been where it is for a long time, but it is still not content.

dshanske commented 6 years ago

The length of time it has been there is why I started the issue to discuss.

grappler commented 6 years ago

I know it has been where it is for a long time, but it is still not content.

Sorry I was not verbose enough. 😞 I wanted to document the fact in the issue so that future me would could just read the comment without having to find the commit again. As it was part of the initial commit means that there are no issues or pull requests with reasoning why it should be added within entry-content.

I have been looking for examples in the wild that place wp_link_pages() differently but it seems like everyone is copying one of the earlier implementations.

I have been playing around with the idea and I am not fully convinced. I see wp_link_pages() as similar to the dots under a slider to switch to a specific slider. The page numbers are specific to the content.

I don't see wp_link_pages() as the same as the_post_navigation().

miklb commented 6 years ago

My 2¢ is that wp_link_pages() is a <nav> element. In general I wouldn't write:

<article>
<p>content</p>
<nav>
stuff
</nav>
</article>

If not a nav then at minimum it would be a footer of the article which still wouldn't put it in the entry-content div.

EDIT I'm making the assumption of a nav element based on the def of wp_link_pages "Displays page links for paginated posts"

Ruxton commented 6 years ago

Everyone puts it in the content, because it's always been there but from a structured data perspective, it arguably belongs outside the content block. I'd argue based purely on semantics it belongs in the footer, as a nav element.

also, wp_link_pages should cause <link rel="next" and <link rel="prev" to get added to head with appropriate pages.

peiche commented 6 years ago

@grappler I don't know if linking to other starter themes is a no-no (if it is I apologize), but Sage 9 puts wp_link_pages outside the content.

<div class="entry-content">
  @php the_content() @endphp
</div>
<footer>
  {!! wp_link_pages(['echo' => 0, 'before' => '<nav class="page-nav"><p>' . __('Pages:', 'sage'), 'after' => '</p></nav>']) !!}
</footer>
grappler commented 6 years ago

Thank you @peiche for the link to Sage 9. It is interesting to see how other people do it.

I have created a PR #1304 to fix this. What do you think?

justintadlock commented 6 years ago

Not that I use this particular theme, but I thought I'd chime in here since I came across this topic.

wp_link_pages() may not be "content" in and of itself, but it's intricately tied to the actual content. It outputs the pagination of the post content. A better name for the function would've been post_content_pagination().

The <!--nextpage--> quicktag triggers this, which is retrieved from $post->post_content.

Depending on the theme design, where to place the call is going to change. I don't see enough reason to break from convention here.

miklb commented 6 years ago

that's what the article footer is for. Semantics > convention