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

Suggested improvements #15

Closed MikePfaff closed 7 years ago

MikePfaff commented 9 years ago

I suggest the following improvements to make it easier to reorganise code (moving sightly instructions around, etc.) and also to improve readability. These suggestions come from my firsthand experience on working with Sightly on a large AEM project. There you often need to reorganise code and change control flow, which is way easier, when these things are separate tags instead of sticking everything on one HTML tag. Also, readability gets really bad with the currently suggested rules. As an example, we have components on our project with several use classes on the root element, plus we also have a test statement on the root element and finally we also have some special CSS classes on this root element which are generated by the use classes.

4.2 - Do not put the use tags on the regular root element. Instead, i would put them in a separate sly tag at the top of the file. Also, i would put a separate sly tag for each use js/java class. In that way it is easier to change the application structure without having to move attributes around, creating tags, etc.:

<sly data-sly-use.teaser="com.example.FirstComponent" />
<sly data-sly-use.somethingElse="com.example.SecondComponent" />
<section class="teaser">
    <h3>${teaser.title}</h3>
</section>

4.7 and 4.8 - I suggest to actually put control statements on separate sly tags to disentangle the control logic from the actually output HTML. In that way it is easier to change the control / flow of the execution without having to move attributes around:

<sly data-sly-test="${teaser.text}">
<p class="teaser__text"></p>
</sly>
kwin commented 9 years ago

Both changes will lead to additional blank lines. That makes the HTML larger (although only slightly ;-)), but in some cases it will actually also affect the rendering of the page. All of that will be solved, once https://issues.apache.org/jira/browse/SLING-4443 is implemented. Please also compare with #14.

MikePfaff commented 9 years ago

As mentioned in #14, if the newline is a problem, i usually wrap it in a Sightly comment.

ErikGrijzen commented 9 years ago

We thought about placing all the use statements on the top of your Sightly scripts, kind of like in real programming languages. Besides the blank lines, the other thing I don't like about this approach, is that the Sightly logic will try to render even if the JS/JAVA class is broken. I personally prefer to see nothing rendered in the HTML.

If the length of your root element is getting out of hand, you could always do something like this:

<section data-sly-use.teaser="com.example.FirstComponent"
         data-sly-use.somethingElse="com.example.SecondComponent"
         class="teaser teaser--$[teaser.someClass}">
    <h3>${teaser.title}</h3>
</section>

What do you think about writing it that way?

About the control flow statements. It can also increase the indentation level of your Sightly scripts, which might hurt the readability in complex components.

kwin commented 9 years ago

Besides the blank lines, the other thing I don't like about this approach, is that the Sightly logic will try to render even if the JS/JAVA class is broken. I personally prefer to see nothing rendered in the HTML.

This is not true IMHO. What do you mean by broken? data-sly-use will only instantiate the class (even as pojo). Usually there are only exceptions being thrown when calling some methods on the class. I don't think the wrapping make any difference in terms of error handling, because either there is an exception being thrown from the sightly script or the exception is suppressed (not leading to the suppression of the whole tag though).

ErikGrijzen commented 9 years ago

@kwin You're right! I just did some tests. I thought not wrapping your logic inside a data-sly-usestatement can lead to some corrupted HTML, like:

<section ></section>

Where the extra space after section breaks the whole DOM tree. But this is not the cause of that problem. My mistake!

gabrielwalt commented 9 years ago

@MikePfaff, the writing style you propose goes against one of the core objectives of Sightly, that the HTML elements of the template should as much as possible correspond to the HTML elements of the output, because this would dilute the markup with countless <sly> elements. If the goal were to keep the markup and the template logic separate, then Sightly wouldn't have been designed to reuse the grammatical elements of the HTML language.

While this can be tolerated in your first example, for data-sly-use statements specifically, in order to kind of highlight them and to isolate them at the top of the file, this practice clearly should be avoided for other statements like data-sly-test or data-sly-list.