-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SHS-5988: Fix empty heading on postcards without title #1699
base: develop
Are you sure you want to change the base?
SHS-5988: Fix empty heading on postcards without title #1699
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mariannuar LGTM!
@ahughes3 Ready for you to review
@ahughes3 Please take a look at this comment before approving the PR. The Chemistry site is not in Tugboat, but the existing links in the cards break after applying this fix. I think that the fix is still necessary, but descriptive titles should be added if they want to keep the links working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
if (!empty($variables['title'])) { | ||
$title = is_array($variables['title']) ? $renderer->renderInIsolation($variables['title']) : $variables['title']; | ||
$variables['title'] = ['#markup' => trim(strip_tags($title))]; | ||
$variables['title'] = $title ?? ['#markup' => trim(strip_tags($title))]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this work? I'm not really understanding.
if $variables['title']
is a render array, we get the rendering, otherwise it's would be a string. But either way $title
is not null/false, so it would always set the value to what is returned in line 187. The ?? ....
doesn't really do anything at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that in line 187 $title
returned an empty string. So in line 188, if we don't add the ??
it will set $variables['title'] like this:
$variables['title'] will return an array in these scenario, so in the template vertical-card.html.twig
it will seem that there's something in title
even though it's an array with an #markup
that is empty. This will add the h2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Verified that vertical postcards without titles do not display an empty H2 tag.
READY FOR REVIEW
Summary
Fix empty heading on postcards without title
Urgency
medium
Steps to Test
Vertical Postcards with Title, Image and Link in a Collection
andVertical Postcards with Title and Image in a Collection
(first of the third row of cards)PR Checklist