Problem/Motivation

If a node is created with title "0" then it doesn't show up in olivero theme. It was fixed in #578400: Display the page title, even if "0" for Drupal 7.

Steps to reproduce

Use olivero theme and create a new node with title as "0". Title is not displayed.

Proposed resolution

I think the problem is with the statement {% if title|render|striptags|trim %} in core/themes/olivero/templates/content/page-title.html.twig which returns '0' if the title is '0' and it's considered as false and the title is not displayed.

Remaining tasks

Follow up, see #20 #21 was fixed in MR.

User interface changes

Before

before

After

after

API changes

NA

Data model changes

NA

Release notes snippet

NA

CommentFileSizeAuthor
#18 Screenshot 2024-02-06 135543.png74.36 KBnitin shrivastava
#18 Screenshot 2024-02-06 135552.png65.95 KBnitin shrivastava
#14 BasicPageAfterMR!4437.png232.46 KBdivya.sejekan
#14 ArticleAfterMR!4437.png263.83 KBdivya.sejekan
#14 BeforeMR!4437.png256.33 KBdivya.sejekan
#10 before-3376566-MR-8.png1.07 MBdjsagar
#10 After-3376566-MR-8.png1.16 MBdjsagar
Screenshot 2023-07-24 at 6.03.59 PM.png70.19 KBkunal.sachdev

Issue fork drupal-3376566

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

kunal.sachdev created an issue. See original summary.

kunal.sachdev’s picture

kunal.sachdev commented
Title: Display the page title, even if "0" in claro and olivero » Display the page title, even if "0" in olivero
Issue summary: View changes

Rechecked it and found that title is shown in claro but not in olivero, hence updating the issue summary and title.

sakthi_dev made their first commit to this issue’s fork.

sakthi_dev opened merge request !4473

sakthi_dev’s picture

sakthi_dev commented

As the title field of a node is mandatory by default, removed the if condition to check the value of that. The reason that the title not rendering is due to the if condition as the value is 0 which is considered as FALSE.
Please review.

kunal.sachdev’s picture

kunal.sachdev commented
Related issues: +#3370946: Page title should contextualize the local navigation
kunal.sachdev’s picture

kunal.sachdev commented
Title: Display the page title, even if "0" in olivero » Display the page title, even if "0" in claro and olivero
Issue summary: View changes

_

kunal.sachdev’s picture

kunal.sachdev commented
Title: Display the page title, even if "0" in claro and olivero » Display the page title, even if "0" in olivero
Issue summary: View changes
kunal.sachdev’s picture

kunal.sachdev commented
Status: Active » Needs review
djsagar’s picture

djsagar commented
StatusFileSize
new After-3376566-MR-8.png1.16 MB
new before-3376566-MR-8.png1.07 MB

@kunal.sachdev i also find the same issue.

Follow the steps which you provided.

After the MR #8, issue has been resolved.

For reference:
Before MR
before MR

After MR
after MR

RTBC ++

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Issue summary is incomplete.

Left some comments on MR.

kunal.sachdev’s picture

kunal.sachdev commented
Issue summary: View changes
kunal.sachdev’s picture

kunal.sachdev commented
Status: Needs work » Needs review
divya.sejekan’s picture

divya.sejekan commented
StatusFileSize
new BeforeMR!4437.png256.33 KB
new ArticleAfterMR!4437.png263.83 KB
new BasicPageAfterMR!4437.png232.46 KB

Verified issue with new changes. Working as expected after MR!4437 .
Attaching Screenshots of results

Testing steps :
1. Use Olivero theme
2. Create a new node with title as "0". Title is not displayed.

Moving to RTBC++

divya.sejekan’s picture

divya.sejekan commented
Status: Needs review » Reviewed & tested by the community
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

The MR question was not answered.

sakthi_dev’s picture

sakthi_dev commented
Status: Needs work » Needs review

Thanks @longwave.
Added the condition as title|render|striptags|trim is not empty so that if there are any tags it will remove it and check for the condition if it is empty. I think this is sufficient. Please review.

nitin shrivastava’s picture

nitin shrivastava commented
StatusFileSize
new Screenshot 2024-02-06 135552.png65.95 KB
new Screenshot 2024-02-06 135543.png74.36 KB

This MR applied successfully. Now the title is visible, and it is rendering fine.

The term "|render|striptags|trim" works well in twig.

After
Example

Before
Example

Moving to RTBC ++

nitin shrivastava’s picture

nitin shrivastava commented
Status: Needs review » Reviewed & tested by the community
quietone’s picture

quietone commented
Component: theme system » Olivero theme
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

I'm triaging RTBC issues. I read the IS and the comments. I don't see any unanswered questions.

This issue is about olivero, I am changing the component.

For those adding screenshots, it is best to include a link in the issue summary so that everyone working on the issue can find the latest screenshots.

I then read the MR and it is good to see a thorough tests, and for multiple themes. This now has a dataprovider to test one page for three themes. To me, that is not a good use of resources. The testing could be done in a loop in the original test thus keeping the setup to one functional test. Perhaps, we leave this as is and add a followup to convert this test class to one method that uses a data provider. Adding tag for a followup.

There is also work in the MR.

kunal.sachdev’s picture

kunal.sachdev commented
Status: Needs work » Needs review

#20: Instead of creating a follow-up to do testing in a loop I changed the test in this issue only.

smustgrave’s picture

smustgrave commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Took the screenshots from #14 to the issue summary.

Believe feedback has been addressed,

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

test should be with other olivero-specific tests in \Drupal\FunctionalTests\Theme\OliveroTest or a file next to it.

kunal.sachdev’s picture

kunal.sachdev commented
Status: Needs work » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

feedback in MR

kunal.sachdev’s picture

kunal.sachdev commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems the use of a bae class has been implemented.

  • nod_ committed 00432f54 on 11.x 
    Issue #3376566 by kunal.sachdev, sakthi_dev, divya.sejekan, Nitin...

  • nod_ committed e5081e91 on 10.3.x 
    Issue #3376566 by kunal.sachdev, sakthi_dev, divya.sejekan, Nitin...

  • nod_ committed ebd60674 on 10.2.x 
    Issue #3376566 by kunal.sachdev, sakthi_dev, divya.sejekan, Nitin...

nod_ closed merge request !4473

nod_ credited catch.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 00432f5 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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