Skip to content
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

Add attributes variable to templates #2292

Closed
wants to merge 3 commits into from
Closed

Conversation

koertho
Copy link

@koertho koertho commented Sep 14, 2020

Q A
Address https://contao.slack.com/archives/CK4J0KNDB/p1599547582289200

This litte PR adds an attributes variable to the body tag to add custom (data) attributes to the body tag without the need to complete override the fe_page template. As this is a very small addition, I would love to see this also merged in the contao 4.9 and 4.4 branch.

Usage example:

/**
 * @Hook("generatePage")
 */
class GeneratePageListener
{
    public function __invoke(PageModel $pageModel, LayoutModel $layout, PageRegular $pageRegular): void
    {

        $pageRegular->Template->attributes = $pageRegular->Template->attributes
            ? $pageRegular->Template->attributes .' data-custom-attribute="myCustomAttribute"'
            : 'data-custom-attribute="myCustomAttribute"';
    }
}

Update:
After discussion this pull request also adds the attributes variable to block templates and all module and element template not inheriting from block templates.

Copy link
Contributor

@fritzmg fritzmg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in favor of this - however I would like this to be expanded to other templates as well, e.g.

  • block_searchable
  • block_unsearchable

and any template that does not inherit from the block templates, e.g. ce_headline for example.

<<?= $this->hl ?> class="<?= $this->class ?>"<?= $this->cssID ?><?php if ($this->style): ?> style="<?= $this->style ?>"<?php endif; ?><?= $this->attributes ?>>
  <?= $this->headline ?>
</<?= $this->hl ?>>

Although technically you could (mis)use $this->cssID for that purpose in those templates.

@@ -21,7 +21,7 @@
<?php $this->endblock(); ?>

</head>
<body id="top"<?php if ($this->class): ?> class="<?= $this->class ?>"<?php endif; ?><?php if ($this->onload): ?> onload="<?= $this->onload ?>"<?php endif; ?> itemscope itemtype="http://schema.org/WebPage">
<body id="top"<?php if ($this->class): ?> class="<?= $this->class ?>"<?php endif; ?><?php if ($this->onload): ?> onload="<?= $this->onload ?>"<?php endif; ?> itemscope itemtype="http://schema.org/WebPage"<?php if ($this->attributes): ?> <?= $this->attributes ?><?php endif; ?>>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<body id="top"<?php if ($this->class): ?> class="<?= $this->class ?>"<?php endif; ?><?php if ($this->onload): ?> onload="<?= $this->onload ?>"<?php endif; ?> itemscope itemtype="http://schema.org/WebPage"<?php if ($this->attributes): ?> <?= $this->attributes ?><?php endif; ?>>
<body id="top"<?php if ($this->class): ?> class="<?= $this->class ?>"<?php endif; ?><?php if ($this->onload): ?> onload="<?= $this->onload ?>"<?php endif; ?> itemscope itemtype="http://schema.org/WebPage"<?= $this->attributes ?>>

I think it should be without a space, as the same concept is used in a few other templates already.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it in my latest commit.

@asaage
Copy link

asaage commented Sep 15, 2020

There would still be a subpalette or wizard required to make use of that right?
I suggested that a long time ago but can't find the issue anymore.
The response was more or less: "make it an extension!" (which i never did)
image

@fritzmg
Copy link
Contributor

fritzmg commented Sep 15, 2020

There would still be a subpalette or wizard required to make use of that right?

Not necessarily. I'd just use it in an parseTemplate hook.

@koertho
Copy link
Author

koertho commented Sep 15, 2020

@fritzmg Should I adjust the other template in this pull request or an follow up?

@fritzmg
Copy link
Contributor

fritzmg commented Sep 15, 2020

Just do it in this one :)

@koertho koertho changed the title Add attributes variable to fe_page template in body tag Add attributes variable to templates Sep 16, 2020
@koertho koertho requested a review from fritzmg October 7, 2020 08:04
@fritzmg
Copy link
Contributor

fritzmg commented Oct 7, 2020

@contao/developers wdyt?

@m-vo
Copy link
Member

m-vo commented Oct 7, 2020

I think the name $attributes might be a bit misleading as the other things like $class, $style, $onload, … are also attributes. Also - without knowing the details - I'd assume that they contain all attributes (probably as an array and not already rendered). Wdyt?

Speaking of the last: I think we shouldn't introduce more precompiled things to the templates. Associative data should imo be in an array. If it gets to cumbersome to output there can still be a template function. Similar story in the Figure: old/prerendered vs new/expanded in template.

Idk, brainstorming:

$pageRegular->Template->attributes['data-custom'] = 'customThing';
<?php echo $this->expand($this->attributes); ?>                // data-custom="customThing" foo="f" bar="b"
<?php echo $this->expand($this->attributes, 'foo', 'bar'); ?>  // foo="f" bar="b"

@fritzmg
Copy link
Contributor

fritzmg commented Oct 7, 2020

I think the name $attributes might be a bit misleading as the other things like $class, $style, $onload, … are also attributes. Also - without knowing the details - I'd assume that they contain all attributes (probably as an array and not already rendered). Wdyt?

While I agree, the fact is that it is already used this way in Contao. This PR would just expand that concept to all other templates.

Speaking of the last: I think we shouldn't introduce more precompiled things to the templates. Associative data should imo be in an array. If it gets to cumbersome to output there can still be a template function. Similar story in the Figure: old/prerendered vs new/expanded in template.

Idk, brainstorming:

$pageRegular->Template->attributes['data-custom'] = 'customThing';
<?php echo $this->expand($this->attributes); ?>                // data-custom="customThing" foo="f" bar="b"
<?php echo $this->expand($this->attributes, 'foo', 'bar'); ?>  // foo="f" bar="b"

👍 :)

Base automatically changed from master to 4.x January 18, 2021 15:42
@leofeyer leofeyer added the stale label Nov 9, 2021
@stale stale bot closed this Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants