Problem/Motivation

Go to "admin/content", you will see 'Content' as simple text, which is the title of the view.
For a page, the title though already appears as <h1>-tag, so we don't need it here.

See screenshot:
Shows the admin-content view region with the title

Proposed resolution

Remove it for page titles.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it produces a bad looking admin UI
Issue priority Major because pretty much every site builder will see and get a little bit annoyed by it.
Unfrozen changes Unfrozen because it only changes markup ;)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

FileSize
33.6 KB

Here is a screnshot, seriously twig people.

damiankloip’s picture

FileSize
621 bytes

So should we just do this for now? or...? If not, why don;t we just remove the title from the template and tweak the views UI preview or something?

damiankloip’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2345343.patch, failed testing.

dawehner’s picture

Title: view TITLE appears all the time. (TWIG :( ) » view TITLE appears all the time.

.

dawehner’s picture

So should we just do this for now? or...? If not, why don;t we just remove the title from the template and tweak the views UI preview or something?

The title is just not rendered for some cases. For example for attachments the title itself is rendered.

olli’s picture

damiankloip’s picture

Sorry, I don't think we should do that.

damiankloip’s picture

FileSize
1.22 KB

So me and Daniel agree that we should just remove this from the template, as it is always set to empty unless you are using preview, it's not needed/relevant in the preview. If you want a title you would need to add this yourself anyway, so might as well leave people to do this?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice!

star-szr’s picture

seriously twig people.

:(. The template hasn't been touched since #1843744: Convert views/templates/views-view.tpl.php to twig , for the record.

olli’s picture

it is always set to empty unless you are using preview

This is also how it is currently documented in the template.

it's not needed/relevant in the preview

So now it is not possible to preview the title?

+++ b/core/modules/views/templates/views-view.html.twig
@@ -38,11 +38,6 @@
-  {{ title_suffix }}

I think this is sometimes needed for contextual links.

damiankloip’s picture

So now it is not possible to preview the title?

You can see the view title on the UI preview anyway, there is a 'title' part above the actual rendered view. So we are currently just duplicating that anyway?

I think this is sometimes needed for contextual links.

afaik, this should still work fine.

Good spot with the comments, I forgot about those. If we do do this, they need to go too.

olli’s picture

there is a 'title' part above the actual rendered view

Aah, you're right, I missed that one!

So we are currently just duplicating that anyway?

Kind of yeah, except that the title inside the preview area used to have an edit link (that does not work/is hidden atm).

afaik, this should still work fine.

Seems that the title_suffix is used for embed display's contextual links.

The title itself is indeed not visible on attachments or embeds, only in preview. If we really don't need it, let's remove it. If we'd want something like #6, maybe we could (in views_preprocess_view) ask the display if title should be visible or not, and leave views_ui_preprocess_view like it is now.

Changing status for those comments in template (and some tests maybe..).

olli’s picture

Status: Reviewed & tested by the community » Needs work
damiankloip’s picture

Kind of yeah, except that the title inside the preview area used to have an edit link (that does not work/is hidden atm).

Yes, but as far as we know, this has been broken for some time. Seems like we should either fix this feature, or drop it?

I am not sure now. But my original patch would keep the behaviour we had before but working with the new changes for #pre_render.

Also, Maybe we should just have this title behaviour for embed? I am just not sure anymore :)

olli’s picture

my original patch would keep the behaviour we had before but working with the new changes for #pre_render.

That is exactly what I thought about my #7 =). I guess the problem with #2 is that it removes the title always, even when it is needed for page title.

damiankloip’s picture

Well, I think #2 preserves the same behaviour we had before? Just the views ui stuff has moved to views_ui_preprocess_views_view?

i.e.

$variables['title'] = !empty($view->live_preview) ? Xss::filterAdmin($view->getTitle()) : '';

Will always override the title anyway.

dawehner’s picture

True, for now the solution is fine.

damiankloip queued 2: 2345343.patch for re-testing.

The last submitted patch, 2: 2345343.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
510 bytes

So this is like #2 kind of, but works.

Status: Needs review » Needs work

The last submitted patch, 22: 2345343-22.patch, failed testing.

Status: Needs work » Needs review

damiankloip queued 22: 2345343-22.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 22: 2345343-22.patch, failed testing.

Status: Needs work » Needs review

damiankloip queued 22: 2345343-22.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 22: 2345343-22.patch, failed testing.

Status: Needs work » Needs review

olli queued 22: 2345343-22.patch for re-testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

For now I am fine with this. Feel free to disagree.

damiankloip’s picture

Agree. This is what we were doing before.

olli’s picture

Yep, I agree.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

A comment would be nice.

Bès’s picture

Status: Needs work » Needs review
FileSize
726 bytes

Try to add comment from what I understood in this issue.

dawehner’s picture

Issue summary: View changes
FileSize
16.7 KB

.

dawehner’s picture

Issue tags: +Needs tests

This was RTBC already, though I kinda thing we should add a quick test somewhere to ensure that this is actually fixed.

damiankloip’s picture

Issue tags: -Needs tests
FileSize
1.89 KB
2.6 KB

ok, here is a quick test (Test only patch is also the interdiff).

Bès, I think maybe the text should be something like:

Override the title to be empty by default. For example, if viewing a page view, 'title' will already be populated in $variables. This can still be overridden to use a title when needed. See views_ui_preprocess_views_view() for an example of this.

As this is not specific to the views preview, and it is there to override the default that is populated for the page. Any thoughts on that?

The last submitted patch, 36: 2345343-36-tests-only-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 36: 2345343-36-PASS.patch, failed testing.

olli’s picture

#36 describes the problem, sounds good. It also made we wonder if we could change page and block views' render array so that the page/block title does not leak into $variables.

jibran’s picture

+++ b/core/modules/views/src/Tests/Plugin/DisplayPageWebTest.php
@@ -121,4 +123,15 @@ public function testPageDisplayMenu() {
+    $xpath = $this->xpath('//div[contains(@class, "view") and contains(., :title)]', [':title' => $view->getTitle()]);

Can we use CssSelector here?

damiankloip’s picture

jibran, I don't think so. How do you get an element based on content provided with a css selector?

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
2.6 KB

Let's try those patches again. Looks like I didn't rebase my local branch.

The last submitted patch, 42: 2345343-42-tests-only-FAIL.patch, failed testing.

olli’s picture

@damiankloip: do you want to change the comment like you proposed in #36? If you want cssSelect, try 'div.view:contains("'. $title .'")'.

damiankloip’s picture

FileSize
2.62 KB
1.66 KB

Sure, here we go. I didn't realise jquery selectors were fair game too, 'css selectors' is a bit misleading now :)

jibran’s picture

Status: Needs review » Reviewed & tested by the community

'css selectors' is a bit misleading now

I don't think so it make a lot more sense now.

damiankloip’s picture

No, I mean the term in general. Not the code here :) that is a psuedo selector that only really exists in jquery?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b4c0a16 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed b4c0a16 on 8.0.x
    Issue #2345343 by damiankloip, dawehner, olli, Bès: view TITLE appears...
jbrown’s picture

Thanks for fixing this!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.