Netcentric / aem-htl-style-guide

A style guide for the HTML Template Language (HTL), the templating language use by the Adobe Experience Manager (AEM).
MIT License
138 stars 50 forks source link

Clarify "4.2 Try to place use data-sly-use statements only on root elements." #22

Closed karollewandowski closed 7 years ago

karollewandowski commented 7 years ago

I'm not sure if I correctly understand rule 4.2. What does "root element" mean? Is it just top level element:

<div> <!-- this is root element -->
    <span>test</span>
</div>
<section> <!-- this is root element -->
    lorem ipsum
</section>

or first top level element in file?

<div> <!-- this is root element -->
    <span>test</span>
</div>
<section> <!-- this is NOT root element -->
    lorem ipsum
</section>
ErikGrijzen commented 7 years ago

Hi @karollewandowski ,

Good point. It's indeed not so clear. I think the important part here is that you don't use data-sly-use statements on deeply nested elements, like this:

<section>
    <div>
        <h1 data-sly-use.teaser="com.example.TeaserComponent">lorem ipsum</h1>
    </div>
</section>

Because in big templates and components this can lead to hard to maintain code, since it's hard to spot which indentifiers are available/used.

I think it's fine to use data-sly-use statements on multiple root elements like this:

<section data-sly-use.header="com.example.HeaderComponent">
    ...
</section>

<section data-sly-use.navigation="com.example.NavigationComponent">
    ...
</section>

So that means we have to update the style guide and rename "root elements" to "top-level elements" to be more specific and clear.

What do you think?

kwin commented 7 years ago

In 99% of the cases each HTL/Sightly script should have exactly one top-level element (to assure that AEM can render a highlighting box around it for reaching the edit dialog on the author). Only for non-maintainable components or nested script files (included from some other script of the same component) there might be more than one top-level element being used in a script.

karollewandowski commented 7 years ago

Hi @ErikGrijzen and @kwin, I think that "top-level elements" is much better name for it in current form, but the more I think about this rule, the less I'm convinced that it's a good practise. I agree with @kwin, that it's good to have one root element and having it in mind rule 4.2 is actually about putting all declarations in root element. It's good in terms of clean coding, but templates are often complex and require few use objects that are sometimes needed only when specific condition is met. If we put all declarations on the only root element, then all of them will be initialized regardless of actual needs. It causes unnecessary resources usage what affects performance. Sometimes template can be divided to smaller ones included when needed and having required use objects initializations, but it's not always possible or worth. I would consider deprecation of this rule and adding new one about trying to have one root element.

Edit: After clicking "Comment" button I realized that rule states "Try to...", so my suggestion about deprecation/removal can be ignored :)

ErikGrijzen commented 7 years ago

Seems like we came to the conclusion the current naming is correct after all.

@karollewandowski can I close this issue?

karollewandowski commented 7 years ago

I still think that "top-level element" is better name. Trying to have single top-level element should be another practise.

ErikGrijzen commented 7 years ago

I agree. 4.2 is now updated to "top-level elements" to avoid any confusion.

Thank you for your thoughts and contribution.