AdmitHub / admithub-telescope-theme

2 stars 2 forks source link

remove empty nav <li>'s when logged out #21

Closed jordantomax closed 9 years ago

yourcelf commented 9 years ago

Is there any problem currently being caused by the <li>'s? Removing them would be a challenge. Currently, we have a top level menu container that does something like this:

<ul>
   {{#each navItems}}
     <li>{{ navItem }}</li>
   {{/each}}
</ul>

So to make the li's disappear when the navItem decides it is empty, we either have to move the li into the template for each item (thus complecting the markup of the nav container and the items -- the children would never be usable outside a ul/ol context, and we'd have to change all the children if we changed the parent), or duplicate logic in the parent so that it knows whether or not the child will be empty.

If the empty li's are causing a problem with styling or rendering, we can do one of those things -- but as long as the only problem is the under-the-hood aesthetics of useless DOM I'd rather just leave it as is, since it keeps the implementation simpler.

jordantomax commented 9 years ago

It's causing problems with styling, but I can probably just become less general in my styles for the header. That sounds like too much work, closing this ticket.

yourcelf commented 9 years ago

Another approach: switch from ul/li to more generic markup like div/span. Then we can do this:

<div class='nav'>
    {{#each navItems}}
      {{ navItem }}
    {{/each}}
</div>
jordantomax commented 9 years ago

Let me think about that. I'll have to take a look, but I don't think that would help.