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

Rethink the way regions are created in View #3417

Open
JSteunou opened this issue Aug 8, 2017 · 9 comments
Open

Rethink the way regions are created in View #3417

JSteunou opened this issue Aug 8, 2017 · 9 comments

Comments

@JSteunou
Copy link
Contributor

JSteunou commented Aug 8, 2017

From #1971 & #3389 it seems that a good clean up for view's regions would be to create those only when needed / accessed.

What about stop create regions from within View constructor, but rather create each one when being accessed, from the getter? Also the region.el should be created from the view el, and not being looked up from some parentEl option, unless the el is already a node or a region without parent view. This would solve the missing region.$el case.

@paulfalgout
Copy link
Member

I completely agree with this. I don't think regions should do any selector work. I'm not convinced we'll want this in v4... but maybe.

@JSteunou
Copy link
Contributor Author

JSteunou commented Aug 8, 2017

To much work as a breaking change?

@paulfalgout
Copy link
Member

I think our major releases shouldn't require much app refactoring.. I think hopefully v4 can be NextCollectionView replacing CollectionView.. whatever we do with CompositeView.. and possibly extracting AppRouter among a few other small breaking changes.. perhaps there's a small breaking change for a region we could add that opens the door to doing some of this in v4.x.. but I'm a bit nervous to change too much. it made v2->v3 too difficult

@paulfalgout paulfalgout removed this from the v4.0 milestone Nov 5, 2017
@paulovieira
Copy link
Contributor

paulovieira commented Nov 8, 2017

Somehow related to this issue, here are some ideas to simplify the way regions are declared (not completely new I guess). See also #3497 for context.

1) "anonymous" regions

In this scenario we would use a boolean attribute in the template to mark the elements that will be used as the regions' containers:

<div>
	...
	<nav data-mr-region></nav>
	...
	<div data-mr-region></div>
</div>

There would be no need to use 'regions' option anymore (at least for the anonymous regions):

var ViewA = Mn.View.Extend({
	template: ...
	
	//regions: {   // <-- not needed!
	//	...
	//}
})

The api would be same as in v3, except that the regions don't have a name anymore. We would use the respective index number instead:

// code inside ViewA

this.getRegion(0).showView(new ViewB)
this.getRegion(1).showView(new ViewC)

// or 

this.showChildView(0, new ViewB)
this.showChildView(1, new ViewC)

This would be valid only for standard regions. For custom regions we would use the standard approach.

The main use case for this new way of declaring regions would be for view with only 1 or 2 regions (which for me is quite common). Having to think of a region name everytime I need new one takes effort (plus adding it to the 'regions' option).


2) "automatic named" regions

It's similar to the above, but for those cases where we actually want the region to have a name. I've seen other people proposing this.

<div>
	...
	<nav data-mr-region="left"></nav>
	...
	<div data-mr-region="right"></div>
</div>

As above, no need to use 'regions' option anymore:

var ViewA = Mn.View.Extend({
	template: ...
	
	//regions: {   // <-- not needed!
	//	...
	//}
})

Then we would use the current api. The region name is the value of the "data-mn-region" attribute.

// code inside ViewA

this.getRegion('left').showView(new ViewB)
this.getRegion('right').showView(new ViewV)

// or 

this.showChildView('left', new ViewB)
this.showChildView('right', new ViewC)

@paulfalgout
Copy link
Member

I don't like non-named regions because different renders could produce different results. With named regions a more declarative API is very possible @JSteunou is already doing this with auto-region. I think we can make this easier to support as well through the current UI.

I think the biggest improvements will come from having the view pass the dom element to the region instead of the region querying the DOM itself. And then finding a way to potentially reduce the need for instantiating the region until the first getRegion

But yeah.. I think the job @ui. does for regions has potentially much better alternatives

@paulovieira
Copy link
Contributor

paulovieira commented Nov 8, 2017

I don't like non-named regions because different renders could produce different results.

Can you explain better how can that happen?

the biggest improvements will come from having the view pass the dom element to the region instead of the region querying the DOM itself. And then finding a way to potentially reduce the need for instantiating the region until the first getRegion

Agree 100%. But those are separate issues. My itent here was just to reduce the boilerplate needed to add a new region. Currently we have to

  1. think of a unique identifier to the html element (via "id", "class", "data-region-id", etc)
  2. use that identifier in the regions option
  3. use that identifier in showChildView()

But the important thing is just in 3). 1) and 2) are nothing but boilerplate that could be automated by marionette.

@paulfalgout
Copy link
Member

<div data-mr-region></div>
{{#if isAdmin}} 
<div class="admin-panel">
  <div data-mr-region></div>
  <div data-mr-region></div>
</div>
{{/if}}
<div data-mr-region></div>

As far as the other stuff, I lean towards reducing the opinions of how this could be accomplished and making it easier for others to come up with implementations to make it easier. Maybe there is a best way to do this, but I could see some people wanting to declare regions and possibly even childviews inside their templates.. then I could also see just allowing people to use a selector instead of a named region in showChildView and essentially making a region in the background for that selector.

@JSteunou
Copy link
Contributor Author

JSteunou commented Nov 8, 2017

  1. I also disapprove anonymous region. Never done and already thinks about edge cases that could break into user code. For some it could work, until...
  2. named region is already what I do by overriding Marionette render and getRegion to
    1. add the view cid as data attribute to each data-region element. This fixes the current issue when the view uses a deep find to get the node from a selector and could get the wrong node. This allows the view to have the correct node and init the region with it instead of a selector.
    2. init the region only when querying, either through getRegion or showChildView (which does call getRegion)

@paulfalgout
Copy link
Member

I think we can somewhat create regions on the fly, and I think the v5 plans allow for this with addRegion if someone really wants to do so, but I think it's moderately problematic to create on the fly as it makes worse the concern of querying all of the children #3320 I do think however that the new region has good hooks for making something like autoregion very performant and simple so in some respects this issue may be resolved by the v5 work.

@paulfalgout paulfalgout added this to the v5 milestone Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants