Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

fix(dropdown): support ngIf directive in templates of dropdown menus. #6176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chrisjsherm
Copy link

@chrisjsherm chrisjsherm commented Aug 18, 2016

When specifying a template-url for the dropdown-menu, the use of jQuery's replace has problems with the comments inserted into the DOM by the ngIf directive and results in incomplete removal of the DOM when the dropdown menu is closed. The problem becomes readily apparent when the menu is open and closed multiple times, as the build up of stale DOM elements starts to create an expanding shadow from the dropdown-menu CSS class.

Example code demonstrating the issue:

<div class="btn-group" uib-dropdown>
    <button id="button-template-url" type="button" class="btn btn-primary" uib-dropdown-toggle ng-disabled="disabled">
        Dropdown using template <span class="caret"></span>
    </button>
    <ul class="dropdown-menu" uib-dropdown-menu template-url="dropdown.html" aria-labelledby="button-template-url">
    </ul>
</div>

<script type="text/ng-template" id="dropdown.html">
    <ul ng-if="true" class="dropdown-menu" uib-dropdown-menu role="menu" aria-labelledby="button-template-url">
        <li role="menuitem"><a href="#">True here</a></li>
        <li role="menuitem"><a href="#">Another action in Template</a></li>
    </ul>

    <ul ng-if="false" class="dropdown-menu" uib-dropdown-menu role="menu" aria-labelledby="button-template-url">        
        <li role="menuitem"><a href="#">False here</a></li>
        <li class="divider"></li>
        <li role="menuitem"><a href="#">Separated link in Template</a></li>
    </ul>
</script>

Using jQuery's html and empty functions, combined with only using the list-item elements in the template specified by template-url allows the use of the ngIf directive without issue.

Working example after making the pull request changes:

<div class="btn-group" uib-dropdown>
    <button id="button-template-url" type="button" class="btn btn-primary" uib-dropdown-toggle ng-disabled="disabled">
        Dropdown using template <span class="caret"></span>
    </button>
    <ul class="dropdown-menu" uib-dropdown-menu template-url="dropdown.html" aria-labelledby="button-template-url">
    </ul>
</div>

<script type="text/ng-template" id="dropdown.html">
    <section ng-if="true">
        <li role="menuitem"><a href="#">True here</a></li>
        <li role="menuitem"><a href="#">Another action in Template</a></li>
    </section>

    <section ng-if="false">        
        <li role="menuitem"><a href="#">False here</a></li>
        <li class="divider"></li>
        <li role="menuitem"><a href="#">Separated link in Template</a></li>
    </section>
</script>

@icfantv
Copy link
Contributor

icfantv commented Aug 18, 2016

@wesleycho, comments? I don't know about this one at all. This feels very wrong and could have BC ramifications for users' existing templates.

@wesleycho
Copy link
Contributor

Why not wrap the content with the ng-if with a div? I'd be concerned about this change - it certainly is a breaking change. I'm strongly inclined to reject this as not desired.

@chrisjsherm
Copy link
Author

chrisjsherm commented Aug 19, 2016

I realize it's a breaking change in terms of requiring the removal of the unordered list element from the template. My perspective is that having the template contain only the list items aligns more closely with how other Angular directives behave now that directives' replace attribute is deprecated. It also avoids repeating the unordered list code inside the template.

Additionally, this fix addresses a visual quirk whereby a dropdown menu with no items flashes onto the UI until the request for the template's HTML is retrieved and compiled.

@wesleycho
Copy link
Contributor

I'm sort of in agreement with you in general here - I guess we can schedule this for 3.0.0.

@wesleycho wesleycho added this to the 3.0.0 milestone Aug 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants