Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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:
Proposed resolution
Remove it for page titles.
Remaining tasks
User interface changes
API changes
Beta phase evaluation
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 ;) |
Comment | File | Size | Author |
---|---|---|---|
#45 | interdiff-2345343-45.txt | 1.66 KB | damiankloip |
#45 | 2345343-45.patch | 2.62 KB | damiankloip |
#34 | title.png | 16.7 KB | dawehner |
#33 | 2345343-33.patch | 726 bytes | Bès |
#1 | screen1.png | 33.6 KB | dawehner |
Comments
Comment #1
dawehnerHere is a screnshot, seriously twig people.
Comment #2
damiankloip CreditAttribution: damiankloip commentedSo 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?
Comment #3
damiankloip CreditAttribution: damiankloip commentedComment #5
dawehner.
Comment #6
dawehnerThe title is just not rendered for some cases. For example for attachments the title itself is rendered.
Comment #7
olli CreditAttribution: olli commentedClosed #2343569: View title appearing in page body, here's a patch that reverts a part of #2221433: Clean up views rendering. Move stuff from template_preprocess_views_view(), into a #pre_render callback.
Comment #8
damiankloip CreditAttribution: damiankloip commentedSorry, I don't think we should do that.
Comment #9
damiankloip CreditAttribution: damiankloip commentedSo 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?
Comment #10
dawehnerNice!
Comment #11
star-szr:(. The template hasn't been touched since #1843744: Convert views/templates/views-view.tpl.php to twig , for the record.
Comment #12
olli CreditAttribution: olli commentedThis is also how it is currently documented in the template.
So now it is not possible to preview the title?
I think this is sometimes needed for contextual links.
Comment #13
damiankloip CreditAttribution: damiankloip commentedYou 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?
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.
Comment #14
olli CreditAttribution: olli commentedAah, you're right, I missed that one!
Kind of yeah, except that the title inside the preview area used to have an edit link (that does not work/is hidden atm).
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..).
Comment #15
olli CreditAttribution: olli commentedComment #16
damiankloip CreditAttribution: damiankloip commentedYes, 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 :)
Comment #17
olli CreditAttribution: olli commentedThat 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.
Comment #18
damiankloip CreditAttribution: damiankloip commentedWell, 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.
Will always override the title anyway.
Comment #19
dawehnerTrue, for now the solution is fine.
Comment #22
damiankloip CreditAttribution: damiankloip commentedSo this is like #2 kind of, but works.
Comment #29
dawehnerFor now I am fine with this. Feel free to disagree.
Comment #30
damiankloip CreditAttribution: damiankloip commentedAgree. This is what we were doing before.
Comment #31
olli CreditAttribution: olli commentedYep, I agree.
Comment #32
alexpottA comment would be nice.
Comment #33
Bès CreditAttribution: Bès commentedTry to add comment from what I understood in this issue.
Comment #34
dawehner.
Comment #35
dawehnerThis was RTBC already, though I kinda thing we should add a quick test somewhere to ensure that this is actually fixed.
Comment #36
damiankloip CreditAttribution: damiankloip commentedok, here is a quick test (Test only patch is also the interdiff).
Bès, I think maybe the text should be something like:
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?
Comment #39
olli CreditAttribution: olli commented#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.
Comment #40
jibranCan we use CssSelector here?
Comment #41
damiankloip CreditAttribution: damiankloip commentedjibran, I don't think so. How do you get an element based on content provided with a css selector?
Comment #42
damiankloip CreditAttribution: damiankloip commentedLet's try those patches again. Looks like I didn't rebase my local branch.
Comment #44
olli CreditAttribution: olli commented@damiankloip: do you want to change the comment like you proposed in #36? If you want cssSelect, try
'div.view:contains("'. $title .'")'
.Comment #45
damiankloip CreditAttribution: damiankloip commentedSure, here we go. I didn't realise jquery selectors were fair game too, 'css selectors' is a bit misleading now :)
Comment #46
jibranI don't think so it make a lot more sense now.
Comment #47
damiankloip CreditAttribution: damiankloip commentedNo, I mean the term in general. Not the code here :) that is a psuedo selector that only really exists in jquery?
Comment #48
alexpottCommitted b4c0a16 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation for to the issue summary.
Comment #50
jbrown CreditAttribution: jbrown commentedThanks for fixing this!