Remove views-more.html.twig and replace with #type link render arrays

Tagging.

updated

The main difference that now view is not passed to preprocess (no idea how views_load_more and others works)
Also this link now gets DIV wrapper

@andypost thanks for picking this up!

It's using the same 'more-link' class but that is now on the wrapper div instead of the a tag. This will make styling this link much easier as it won't have to get unnecessary selectors added like a.more-link and div.more-link, so big +1 to this approach!

We need to do a manual test on this, I wonder if there are any views using this option in core, that will help with the manual testing. I don't think so but maybe we can get some steps to reproduce in the Issue Summary.

1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2100,13 +2100,11 @@ public function renderMoreLink() {
   -        $theme = $this->view->buildThemeFunctions('views_more');
   

   This removes the built up suggestions. Can we somehow get this back through hook_suggestions() or something on the render array?

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2100,13 +2100,11 @@ public function renderMoreLink() {
   -          '#more_url' => $url->toString(),
   ...
   +          '#url' => $url,
   

   This $url->toString() to let it print itself needs to be checked in the manual testing but I expect it should work, and if so +1 to that change!

3. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2100,13 +2100,11 @@ public function renderMoreLink() {
   -          '#link_text' => String::checkPlain($this->useMoreText()),
   ...
   +          '#title' => $this->useMoreText(),
   

   This didn't need the check plain to begin with, nice change:)

4. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2100,13 +2100,11 @@ public function renderMoreLink() {
   -          '#view' => $this->view,
   

   No longer has view variable in the context. Is this acceptable?

Check as Needs review

@_nolocation or @dimaro would you mind addressing some of the items in #4?

And thanks for finding the location where we can test @_nolocation! Mind adding that to the Issue Summary?

Meh, I'm a bit late :)
Here is a patch with #view in context and restored view suggestions.

Awesome thanks @m1r1k, this could use manual testing, I moved @_nolocation's note to "Steps to Reproduce" in the issue summary.

Just need a bit of assurance with manual testing: Someone can take a screenshots of the markup before and after.

Some doc links for what those tags mean:
https://www.drupal.org/issue-tags/special#needs-manual-testing
https://www.drupal.org/issue-tags/special#needs-screenshot

Becomes even better :)

Before:

After:

Also now you could create such templates:
containner--more-link[--view-dipslay-id].html.twig or any other common views related suggestion.

@m1r1k, since this is your patch, somebody else should test. :)

Just refactored it a little. Let me know what do you think @m1r1k. Feel free to pay me a beer ;)

@rteijeiro, you are Mr. Makeup! I'm fine with both requests :)

Not ready for RTBC yet, still needs manual testing. Will do that once #16 passes.

Sorry, my bad. The refactor was not as good as I expected so back to previous code from @m1r1k. Just kept the conditions in one line to align with coding standards. Also provided screenshots.

More Link BEFORE

More Link AFTER

While I like the formatting of the "after" link more than the "before," I don't think we want to be introducing a visual change?

Though maybe it's fixing an error introduced at https://www.drupal.org/node/2031301#comment-8861319? That issue also has a discussion of whether or not the <a> should be wrapped in a <div>, see https://www.drupal.org/node/2031301#comment-9062805.

@akalata I took a look at #2031301: Replace theme_more_link() and replace with #type 'more_link' and can confirm that the full-width link has nothing to do with that issue, checked before and after and the error is not introduced there. Note: the blocks that we're relevant to that issues, weren't view blocks back then.

In this issue we're dealing with a read-more link that's specific to views.

I also looked at before and after #2020393: Convert "Recent content" block to a View. Even directly after the conversion of the recent content block to views the read-more link was still in the wrapper div and not displayed full width.

Currently in head the style shows the version without the wrapper div with a display: block, which makes it display fullwidth. I am about to do a git bisect (http://webchick.net/node/99) to find which commit introduced the new html without the wrapper div, so we can find out if this is a change made on purpose.

So ive been bisecting all afternoon, still can't figure out where the commit which made the change it. It's somewhere between 87fe2f4 (good) and (4b172c7) bad.

Found it: this http://cgit.drupalcode.org/drupal/commit/?id=1b76c015a944617e5edd77a9a2e... is the commit that added the borders to each link. This is done so there is an extra visual indicator for displaying a link, which makes sense. Since the more link is a display: block without a container, it spreads out to the available width. Possible solutions are to alter display to inline-block, but that will make it align to the left, or add the wrapper div previously removed (https://www.drupal.org/node/2226923) again with a text-align: right and an inline-block on the link itself.

Thanks @haasontwerp!

Based on the comments at #2031301-128: Replace theme_more_link() and replace with #type 'more_link', the wrapping <div> was kept in order to position the link without having the dotted underline be full-width. This isn't ideal (see @thedavidmeister's comment in #2031301-138: Replace theme_more_link() and replace with #type 'more_link'), but it's what we have at the moment. I think introducing that bugfix here is okay.

I think it would be good to document on the issue a before and after of the markup changes.

Re #27 @YesCT do you mean or think we need a change record for this?

Thanks for the screenshots @rteijeiro!

Thanks a lot for the screenshots, that's a huge help. Also thanks for the historical digging, hassontwerp. :)

This looks like a great clean-up, and markup is unfrozen, so this is good for beta. I did get confused about why we're doing hook_theme_suggestions() but I see in #4 this was explicitly asked for to retain feature compatibility.

Committed and pushed to 8.0.x. Thanks!

Needs followup

+++ b/core/modules/views/views.module
@@ -279,6 +275,15 @@ function views_theme_suggestions_comment_alter(array &$suggestions, array $varia
+  if (!empty($variables['element']['#type']) && $variables['element']['#type'] == 'more_link' && !empty($variables['element']['#view']) && $variables['element']['#view'] instanceof \Drupal\views\ViewExecutable) {

There should not be full namespace, add "use" statement and it's strange that there's no interface for ViewExecutable

@andypost there you go, thanks for spotting that: #2481751: Don't use full namespace for \Drupal\views\ViewExecutable in views.module