edcarroll / ng2-semantic-ui

Semantic UI Angular Integrations (no jQuery)
https://edcarroll.github.io/ng2-semantic-ui/
MIT License
615 stars 223 forks source link

feat(select): Allow simpler definition of option template #134

Open trevordaniels opened 7 years ago

trevordaniels commented 7 years ago

Instead of:

<sui-select [optionTemplate]="myTemplate">
   <sui-select-option *ngFor="let option of [0,1,2]" [value]="option"></sui-select-option>
</sui-select>

<ng-template let-option #myTemplate>
   {{option + ' years'}}
</ng-template>

could it be possible to have:

<sui-select>
    <sui-select-option *ngFor="let option of [0,1,2]" [value]="option">{{option + ' years'}}</sui-select-option>
</sui-select>
edcarroll commented 7 years ago

Unfortunately this is something that isn't possible. This is because the template is used not only when rendering options in the dropdown, but also when an option is selected and shown when the dropdown is closed, as below:

image

When designing the select, I toyed with making it so that you didn't need a <ng-template> element at all, by copying the innerHTML of the chosen option. This worked well until the options were loaded remotely, as it then required some hacks to make the right option get rendered so we could use the HTML. Therefore, it was easier to stick to an external template.

I hope that all makes sense!

trevordaniels commented 7 years ago

That does make sense. Could it use this inline template when not fetching remotely, but require a separate template otherwise? I ask because selects are something I use a lot and the verbose syntax is clunky, and none of my selects currently fetch remote options.

edcarroll commented 7 years ago

I see where you're coming from, and can see it would be a lot less clunky! I'm trying to think of a way to implement it that isn't confusing and doesn't get affected by edge cases (like custom markup inside the select). Are you happy for me to come back to this when I have had a chance to think more?

trevordaniels commented 7 years ago

Yes, definitely. Thanks for taking another look.

edcarroll commented 7 years ago

In the 0.9.0 release I've added an optionFormatter property to the select component that is a simpler version of a full template. I was wondering if this suits your case here?

trevordaniels commented 7 years ago

This looks good although I probably won't use it as I prefer not to write a separate function just to handle formatting. Nothing beats angular's simple template syntax but I realise that is hard to replicate here. My slightly simpler solution for now is to wrap sui-select in a custom control which passes through a template ref

@ContentChild(TemplateRef) optionTemplate: TemplateRef<any>;

which allows the use of an inline template

<form-select [formControl]="paymentType" [options]="paymentTypes" valueField="id">
    <ng-template let-option>{{option.description}}</ng-template>
</form-select>

But i'd like to simplify it further if possible. It seems that if an options array is specified, specifying the sui-select-option is always the same and almost redundant?

<sui-select-option *ngFor="let o of formatted.filteredOptions" [value]="o"></sui-select-option>
edcarroll commented 7 years ago

I really like that solution of having the template inline. I tried to replicate it in the library earlier but it kept finding comment elements generated by Angular - how did you deal with this?


The final point is tricky, because yes it is slightly redundant, but it also isn't as it allows you to fully customise the menu and where the elements appear (for example you could even group the elements if you didn't mind losing search support).

An example of extensive customisation is the docs 'in-menu search' example:

<sui-multi-select [(ngModel)]="selected" [options]="options" labelField="name" [maxSelected]="5" icon="list" #select>
    <div class="ui icon search input">
        <i class="search icon"></i>
        <input suiSelectSearch type="text" placeholder="Search options...">
    </div>
    <div class="divider"></div>
    <div class="header">
        <i class="list icon"></i>
        Options
    </div>
    <div class="scrolling menu">
        <sui-select-option *ngFor="let o of select.filteredOptions" [value]="o"></sui-select-option>
    </div>
</sui-multi-select>

As in this case the options are nested a level deeper than usual.

One thought I've just had is potentially adding an optional <sui-select-options> element, defined as such:

This would allow a generic select to be:

<sui-select [options]="options"></sui-select>

and the more advanced example above to become:

<sui-multi-select [(ngModel)]="selected" [options]="options" labelField="name" [maxSelected]="5" icon="list" #select>
    ...
    <div class="scrolling menu">
        <sui-select-options></sui-select-options>
    </div>
</sui-multi-select>

Thoughts?

edcarroll commented 7 years ago

FYI, something like this already exists (in 0.9.0) in the [suiSelectSearch] directive.

trevordaniels commented 7 years ago

I really like your proposed change, its clean and clear.

Angular adds a few binding comments but not much, is that what you are referring to? Eg.

<form-select ngdefaultcontrol="" ng-reflect-form="[object Object]" ng-reflect-form-control="[object Object]" ng-reflect-fluid="true" ng-reflect-options="Mr,Mrs,Miss,Ms" class="undefined">
  <sui-select class="selection fluid ui dropdown ng-untouched ng-pristine ng-invalid active visible"   placeholder="Select..." ng-reflect-klass="selection" ng-reflect-ng-class="[object Object]" ng-reflect-options="Mr,Mrs,Miss,Ms" ng-reflect-placeholder="Select..." ng-reflect-form="[object Object]" tabindex="0">
    <input suiselectsearch="" type="text" hidden="" class="search" autocomplete="off">
    <!--bindings={
        "ng-reflect-ng-if": "true"
      }-->
    <div class="default text">Select...</div>
    <div class="text filtered">
      <span></span>
      <!--bindings={}-->
    </div>
    <i class="dropdown icon"></i>
    <div class="menu transition visible" suidropdownmenu="" ng-reflect-menu-transition="slide down" ng-reflect-menu-transition-duration="200" ng-reflect-menu-auto-select-first="false" style="">
      <!--bindings={
        "ng-reflect-ng-for-of": "Mr,Mrs,Miss,Ms"
      }-->
      <sui-select-option ng-reflect-value="Mr" class="item">
        <span></span>
        <span><b></b>Mr</span>
      </sui-select-option>
      <sui-select-option ng-reflect-value="Mrs" class="item">
        <span></span>
        <span><b></b>Mrs</span>
      </sui-select-option>
      <sui-select-option ng-reflect-value="Miss" class="item">
        <span></span>
        <span><b></b>Miss</span>
      </sui-select-option>
      <sui-select-option ng-reflect-value="Ms" class="item">
        <span></span>
        <span><b></b>Ms</span>
      </sui-select-option>
      <!--bindings={
        "ng-reflect-ng-if": "false"
      }-->
    </div>
  </sui-select>
</form-select>
edcarroll commented 7 years ago

Great! I'm currently trying to work out a way to enable this functionality without making it a breaking change - if I can then it will go out in 0.9.3 😄


Yep that's it - every time I try ContentChild(TemplateRef) I just end up with these comment elements, not the <ng-template>... Any ideas?

edcarroll commented 7 years ago

Just wanted to revisit this one - did you manage to successfully make the ng-template inline without running into the issue I mentioned?

Unfortunately things have been busy so I haven't been able to work on this, but it's still on my radar.

trevordaniels commented 7 years ago

Sorry, I dont exactly understand the issue you are having. Can you provide more detail as to what the specific problem is?​

edcarroll commented 7 years ago

So I tried to make it so that I use @ContentChild to access an internal TemplateRef similar to how you described a bit earlier in the comment chain, however every time I tried to run it I was left with Angular picking up comment elements from dev mode, rather than the template I wanted. Does that make sense?

trevordaniels commented 7 years ago

I havent used the template ref in any other way than supplying it directly to the sui-select control, so have not had to deal with this issue of angular's dev comments. It works as expected in this context.​

edcarroll commented 7 years ago

I see - apologies, I was asking as you'd mentioned before that you had wrapped the sui-select in a custom component that passes through a TemplateRef, which is defined inline.

Will keep looking into the other simplifications 😄