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

fix(select): select not rendering option wrapped in ng container #1280

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aefox
Copy link
Contributor

@aefox aefox commented Mar 6, 2019

Please read and mark the following check list before creating a pull request:

Short description of what this resolves:

This fixes issue mentioned in #1063 related to nb-select not rendering nb-options that are wrapped in ng-container

Copy link
Contributor

@yggg yggg left a comment

Choose a reason for hiding this comment

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

As always, thanks for help @aefox! 👍

@@ -28,6 +28,6 @@

<nb-card *nbPortal class="select" [ngClass]="[status, overlayPosition]" [style.width.px]="hostWidth">
<nb-card-body>
<ng-content select="nb-option, nb-option-group"></ng-content>
<ng-content select="nb-option, nb-option-group, ng-container"></ng-content>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's ok to select ng-container here. This way it would be possible to pass arbitrary content into select. What do you think @aefox? Probably there is something we could do to restrict ng-container content (e.g. more specific selector)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yggg I totally see your point on possible to pass arbitrary content into select. That indeed is a problem I did not figure out, nice catch 👍

At the same time, please note the following hack/trick:

<nb-select placeholder="Select Showcase" [(selected)]="selectedItem">
  <ng-container *ngFor="let item of items" ngProjectAs="nb-option">
    <div>{{ item.label }}</div>
  </ng-container>
</nb-select>

This is currently possible to do and it works. Works here means it actually renders the divs correctly (they ofc do not work as an option, but that is expected).

I'm not sure if we should force the users of nebular to restrict them from using ng-container in this situation as probably some devs will prefer to use ng-container for loops and keep their nb-options cleaner.

Indeed doing this we're opening up the possibility for miss usage, but the same can happen for the pure html tags (select and option).

I'm basically opened to doing this any way you think it is best.

@yggg yggg added this to the 3.5.0 milestone Mar 14, 2019
@nnixaa nnixaa removed this from the 3.5.0 milestone Apr 12, 2019
@nu5rim3
Copy link

nu5rim3 commented Jun 25, 2019

any update?

@JihadMotii-REISys
Copy link

Any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants