Plugin-outlet incorrectly handles multiple plugins at the same outlet

Bug
1

Wrong code is:
https://github.com/discourse/discourse/blob/v1.4.0.beta9/app/assets/javascripts/discourse/helpers/plugin-outlet.js.es6#L158

Must be Ember.ContainerView.extend({tagName: ''}) instead of Ember.ContainerView.
The current wrong code breaks HTML markup in cases where HTML syntax does not allow wrapper DIV tag.
An example is admin-menu plugin outlet:
https://github.com/discourse/discourse/blob/v1.4.0.beta9/app/assets/javascripts/admin/templates/admin.hbs#L26

If there are multiple admin-menu connectors then Discourse wrongly renders them as:

<ul>
	<!-- standard menu items: -->
	<li></li>
	<li></li>
	<!-- admin-menu connectors: -->
	<div>
		<li></li>
		<li></li>
	</div>
</ul>
7 Likes
2

I noticed this last May with the hamburger menu’s (aka sitemap) plugin-outlet but never got around to digging into it.

https://meta.discourse.org/t/auto-closing-divs/29159/19?u=mittineague

1 Like
3

What you are asking for here is a container view without a container view… @eviltrout this is gonna be might tricky if we decide to support this.

4

A container view can have a tagName, I just have to pass it through. Should not be too hard.

2 Likes
5

Oops I see what you mean Sam. If I apply the tagname to the ContainerView it’s not what you want, because then you are expecting the children to have li too.

I’m gonna ask people about this. Maybe I can append to the Morph.

1 Like
6

Replacing Ember.ContainerView to Ember.ContainerView.extend({tagName: ''}) as I described in the first message definitely works, I have tested it for my multiple plugins at admin-menu outlet.

7

It didn’t work when I tried it. If you pass the tagName to the ContainerView the container gets the li tag, but then the children get div tags. So adding two <li> doesn’t work.

1 Like
8

Except, I think that is valid HTML now (as of HTML 5). I think li can contain block elements (such as div), so that is less of a problem than the li being wrapped in a div.

9

Untitled-1.png1051×282 23.7 KB 

<li id="ember1719" class="ember-view">  		
	<a id="ember1993" class="ember-view" href="/admin/files">Files</a>
</li>
<li id="ember1724" class="ember-view">  		
	<a id="ember2020" class="ember-view" href="/admin/shop">Shop</a>
</li>

plugins/df-restrict-files/assets/javascripts/connectors/admin-menu/df-files.js.es6

import NavItem from 'discourse/components/nav-item';
export default NavItem.extend({
	_init: function() {
		this.set('route', 'adminFiles');
		this.set('label', 'df.files.title');
	}.on('init')
});

plugins/df-restrict-files/assets/javascripts/connectors/admin-menu/df-files.hbs

{{#if routeParam}}
	{{#link-to route routeParam}}{{i18n label}}{{/link-to}}
{{else}}
	{{#if route}}
		{{#link-to route}}{{i18n label}}{{/link-to}}
	{{else}}
		{{#if path}}
			<a href="{{unbound fullPath}}" data-auto-route="true">{{i18n label}}</a>
		{{else}}
			<a href="{{unbound href}}" data-auto-route="true">{{i18n label}}</a>
		{{/if}}
	{{/if}}
{{/if}}
10

Can you show more of your HTML snippet? I want to see what the <li> tags are wrapped in. I am seeing different results.

11

Wrapped in UL, of course.

12

Any progress on this? I’d be happy if I could get one plugin to render w/o a wrapping div.

13

I think the only way to do this would be to only allow widgets for these cases, @eviltrout would that be doable?

14

Even widgets require a master container div. @nikdavis is your situation the same menu as above? Or elsewhere?

15

Different. I’m working on the user-activity-bottom plugin outlet. The .nav-stacked styling uses a child selector which breaks on the outlet.

16

In theory with a widget container we could use metamorphs optionally for the extend a ul option

1 Like
17

Sure a widget could be mounted with a metamorph or something similar - the problem is you always need a {{mount-widget}} to start hosting them and that needs a tag.

3 Likes
18

A post was split to a new topic: Allow registrations of components as plugin outlets

19

Also, important to note, the OP is resolved here as of latest, so I might as well close and split.

2 Likes