taiga-ui icon indicating copy to clipboard operation
taiga-ui copied to clipboard

[FEATURE] `tui-doc-documentation` poor design resulting in bad DX

Open hakimio opened this issue 3 years ago • 9 comments

🚀 Feature request

Is your feature request related to a problem?

Right now in order to declare tui-doc-documentation child items, you have to use ng-template. This results in long property names like documentationPropertyName and irrelevant auto-completion results by the IDE.

Describe the solution you'd like

Instead of using ng-template to declare tui-doc-documentation children, there should be created a separate component called tui-doc-property, so that developer can use following way to declare the children:

 <tui-doc-documentation>
      <tui-doc-property
          name="closeOthers"
          mode="input"
          type="boolean"
          [(value)]="closeOthers"
      >
          Other sections are closed when user opens one
      </tui-doc-property>
 </tui-doc-documentation>

versus the old ugly way:

 <tui-doc-documentation>
      <ng-template
          documentationPropertyName="closeOthers"
          documentationPropertyMode="input"
          documentationPropertyType="boolean"
          [(documentationPropertyValue)]="closeOthers"
      >
            Other sections are closed when user opens one
      </ng-template>
 </tui-doc-documentation>

Additional context

If you need access to underlying ng-template, you can still get it if you implement tui-doc-property the following way:

import {ChangeDetectionStrategy, Component, TemplateRef, ViewChild} from '@angular/core';

@Component({
    selector: 'tui-doc-property',
    template: `
        <ng-template>
            <ng-content></ng-content>
        </ng-template>
    `,
    changeDetection: ChangeDetectionStrategy.OnPush
})
export class TuiDocPropertyComponent {

    @ViewChild(TemplateRef, { static: true })
    template: TemplateRef<any>;

}

hakimio avatar Sep 03 '21 11:09 hakimio

Pretty much the same applies to tui-doc-page. Proposed design (tui-doc-tab instead of ng-template):

<tui-doc-page
    header="Element" 
    package="CDK" 
    type="directives"
>
    <tui-doc-tab 
        name="Setup"
    >
        <p>Foo Bar!</p>
    </tui-doc-tab>
</tui-doc-page>

versus the current one:

<tui-doc-page
    header="Element" 
    package="CDK" 
    type="directives"
>
    <ng-template
        pageTab="Setup"
    >
        <p>Foo Bar!</p>
    </ng-template>
</tui-doc-page>

hakimio avatar Sep 06 '21 12:09 hakimio

In case of tui-doc-page this would load the tab and keep it in the memory even if you are on a different one. And with single input it makes no sense, so let's keep this about tui-doc-documentation. If you want something shorter it would probably work like this: <div *pageTab="Setup"> :)

waterplea avatar Sep 06 '21 13:09 waterplea

It's not only about how short it is.

  • ng-template tag is ambiguous. By just looking at the tag name, you can't tell what it's supposed to do here.
  • You still get a lot of useless IDE auto-completion results with ng-template as opposed to specific component.

Why wouldn't my proposed solution work for pageTab?

hakimio avatar Sep 06 '21 13:09 hakimio

Let me explain. ng-template is not ambiguous, it means a piece of content that is declared but not instantiated. It is super cheap and take no resources until you create a view based on it. The pattern you suggested in the first message with ng-content under ng-template gets it instantiated and then hidden. That's wrong for tabs. You get proper IDE auto-completion for directives on ng-template from currently imported modules: image

As for your second message, once again, ng-template is a piece of content that you want to instantiate later or at different place, or multiple times. That's what it is made for. Directives on ng-template are basically equivalent to structural directives and it is Angular's way to rearrange layout.

ng-container is used when your directives should not add any elements to layout. It is perfectly fine to use if your directives do not need ElementRef and are used for some dependency injection purposes, for example.

As per your original question, I'm not against refactoring tui-doc-documentation and dropping those long names. But it might be tricky because inside it there's a proper table. So it might be that this component will end up being an attribute selector on <tr> tag and that will not make it much shorter or easier to use. So this has to be properly architectured.

waterplea avatar Sep 06 '21 18:09 waterplea

How does it make sense having tr inside tui-doc-documentation? Or do you want to have sth like the following:

<table tuiDocDocumentation>
    <tr tuiDocProperty>
        <td tuiDocDescription>Foo bar</td>
    </tr>
</table>

which doesn't make much sense to me either...

hakimio avatar Sep 06 '21 18:09 hakimio

ng-template is not ambiguous, it means a piece of content that is declared but not instantiated.

I know what ng-template does. My point is that it is basically a blank slate. It can be template for a tooltip content or as in this case tab template. It does not represent one single thing like, for example, tui-doc-property or tui-doc-tab. Every time you read your component's template, you have to search for those attribute selectors like pageTab or documentationPropertyName to see what this particular ng-template supposed to do.

hakimio avatar Sep 06 '21 19:09 hakimio

BTW, just checked how Angular Material tabs are implemented. They use component selector <mat-tab> and the template looks like this:

<ng-template><ng-content></ng-content></ng-template>

Source

hakimio avatar Sep 06 '21 19:09 hakimio

You get proper IDE auto-completion for directives on ng-template from currently imported modules: image

That's a very simplistic contrived example where the attribute selector is a simple pageTab and you only have one module which provides ng-template directives. Now, let's assume we use proper naming with tuiDoc prefix for the attribute name to avoid collisions and import some other modules like TuiDocDocumentationModule which also provide ng-template directive. Now, you start typing <ng-template tuiDoc and you get some completely irrelevant results.

hakimio avatar Sep 06 '21 19:09 hakimio

I see this conversation is going nowhere so here's the bottom line:

  • current use of templates is idiomatic and it is the way you're supposed to use them
  • documentation component possibly could be and should be simplified, so this issue remains open

As per material tabs, here's a quick demo of why this is suboptimal: https://stackblitz.com/edit/angular-1tensw?file=src%2Fapp%2Ftab-group-basic-example.html You can see that test is printed into console 3 times because all 3 tabs content is initiated, it would follow lifecycles and change detection, even though it is not attached to DOM. If you want to discuss this any further you can DM me over any public channel that is suitable to you.

waterplea avatar Sep 07 '21 07:09 waterplea