OEL-2251: Use heading in featured-media pattern. #307
Conversation
templates/patterns/featured_media/pattern-featured-media.html.twig
Outdated
Show resolved
Hide resolved
| {% include '@oe-bcl/heading' with { | ||
| title: title, | ||
| title_tag: title_tag|default('h2'), | ||
| title_link: title_link|default(''), |
There was a problem hiding this comment.
here we have the choice between an undefined variable and an empty string, does it make sense to set it?
There was a problem hiding this comment.
I removed the default on title_link.
templates/patterns/featured_media/pattern-featured-media.html.twig
Outdated
Show resolved
Hide resolved
tibi2303
force-pushed
the
OEL-2251
branch
2 times, most recently
from
e3d93a2
to
525760a
Compare
templates/patterns/featured_media/featured_media.ui_patterns.yml
Outdated
Show resolved
Hide resolved
| label: 'Title tag' | ||
| description: 'The tag to use for the title. Defaults to h2.' | ||
| title_link: | ||
| type: LinkObject |
There was a problem hiding this comment.
The type must be a php type, like string, array, or an object of a certain class name, this is neither. Let's use string and set whatever format the heading in the template. Then also name it correspondingly, perhaps title_url ?
There was a problem hiding this comment.
Indeed. I also added tests and changed the pattern of gallery. Please have a look. I guess we either change the ticket and title of this PR or we make the gallery changes in another ticket.
| {% include '@oe-bcl/heading' with { | ||
| title: title, | ||
| title_tag: title_tag|default('h2'), | ||
| title_link: title_link, |
There was a problem hiding this comment.
with my previous comment I meant that the entire line can dissapear, but since you added it to the yml, and I dont see why not, then you can add the default back
There was a problem hiding this comment.
It should not only be the default as this param is expecting a BCL link object. I added the check if it's exists and merging it accordingly. Also did this for gallery.
| title: title, | ||
| title_tag: title_tag|default('h2'), | ||
| title_link: title_link, | ||
| attributes: create_attribute({'class': 'mb-4'}).merge(title.attributes|default(create_attribute())), |
There was a problem hiding this comment.
title.attributes is still nok, because title is marked as a string in the yml, and we should keep it that way.. we want to move away from these objects because they cause complications in the drupal layer that limit our solutions.
There was a problem hiding this comment.
it should be cleaned now. Only creating the attribute and adding the class.
tibi2303
force-pushed
the
OEL-2251
branch
2 times, most recently
from
3e47333
to
d728388
Compare
| @@ -12,6 +12,12 @@ | |||
| {% set items = bcl_gallery_items(items|default([])) %} | |||
| {% set icon_path = bcl_icon_path %} | |||
|
|
|||
| {%- if title_url is not empty -%} | |||
There was a problem hiding this comment.
I think its a misunderstanding, let's rather remove all these new fields to prevent opening a pandoras box of heading fields. And later if they are needed it will be a separate ticket handing them all
drishu
changed the title
drishu
changed the title
Jira issue(s):