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

Regions: There and Back Again #1972

Open
jamesplease opened this issue Oct 6, 2014 · 11 comments
Open

Regions: There and Back Again #1972

jamesplease opened this issue Oct 6, 2014 · 11 comments

Comments

@jamesplease
Copy link
Member

Work in progress

This issue is to document my proposal Regions 3.0.

The Motivation

Regions work awesomely in the most common use cases. But there are still a set of features that you would expect to work, but don't. There are three known issues with Regions:

  1. When a LayoutView is created from a tagName, its Regions are not set up until (1) you call show on the region, or (2) you render the layoutView twice. I have no qualms with number one, but number two is just odd. Regions should be available on the first render. In the past we've hand-waved this away as a performance feature, but closer inspection revealed it was added as a means to fix an issue where re-rendering a Layout would not destroy the old views. (In a LayoutView, the region's $el should be available after render #1971)
  2. LayoutViews do not scope the region lookup mechanism within their own element (Regions hash in LayoutView attaches regions that are outside the LayoutView's DOM tree #1685)
  3. When a LayoutView already exists, resetting its regions will mess up the selector. (Layout + Region.reset selector bug #1673)

From my research, none of these issues stem from a single source. Rather, it's the culmination of PRs upon PRs adding new features onto a system that was created from a shaky foundation.

This solution is about a few key changes to the system that will make the code simpler, easier to document and explain, and just as powerful. Do note that this is just an outline, though. Some of the details might need to be adjusted as we go into the implementation phase, but I think the underlying philosophy behind the system is solid.

The only possible negative to this solution is that it depends on the idea that we get rid of the RegionManager class.

Why merge RegionManager into LayoutView?

LayoutViews are the only class that should use the RegionManager logic. They should be in the view, or otherwise a mixin, a la Backbone.Events, that we add to the prototype.

That's not true, though! Applications have a RegionManager!

That's true, but that is a bad feature. Applications should just be containers. Instead of app.addRegion, we should promote creating a LayoutView as the base of the view chain. This LayoutView can use an existing element on the page to begin the view tree from. This is better in so many ways. It reduces the complexity of explaining how things work. You no longer need to even think about how a RegionManager – or the code that it represents – even exists. Marionette will become views all the way down. It's a simple system that's so easy to explain, and much easier for new users to understand.

I'm still not convinced

I did my best :(

The solution

The system proposed is very simple. In a lot of ways, it doesn't change the expectations people have for how Regions work. It only changes the logic behind it, so that the behavior of the system matches up with our expectations better.

The change revolves around an idea that there are two types of LayoutViews: existing ones and pre-render ones. Existing ones come about when you create a LayoutView using an element already on the page, like if you were to specify the el as $('.my-layout-view'). Pre-rendered ones come about when you specify a template, and its Javascript that creates the element. Pre-rendered templates are distinct from existing layouts up until render is called. After that point, this system treats them exactly the same. But it's before that point that matters.

Instantiating the Region

Existing LayoutView

Create the Regions by binding them to nodes within this.el. Use this.$el.find() to scope it.

Pre-render LayoutView

Do nothing with the regions.

Adding a new Region to a LayoutView via addRegion (or whatever method we have)

Existing LayoutView

Create the Regions by binding them to elements just like during the instantiation phase.

Pre-render LayoutView

Do nothing with the Regions, other than adding them to the regions hash.

Rendering the LayoutView for the first time

Existing LayoutView

Re-create the Regions. This could be done in a few ways...In the first way, we do something akin to 'resetting' the Regions in the current code. We re-bind the element that they're attached to. This new system would be different, though, in that we would keep the View that's in the Region around by default (resolving #1263).

In another case, we pick up actual nodes and put them back into the DOM. This would preserve any events on the region's elements, which has its benefits. But it might require introducing documentFragments into the LayoutView to ensure that a re-render is still a single paint...though maybe not. Either way, this feature might not be worth the trouble.

Pre-render view

Render the LayoutView, then create the Regions. At this point, the distinction between existing and pre-render views is gone. They now both have an HTML structure for their element.

Second render

Re-create the regions, as was done in existing layoutview-first render.

In addition to these changes, the regions would be available through a getter to resolve #1821.

@jmca
Copy link
Contributor

jmca commented Oct 9, 2014

we should promote creating a LayoutView as the base of the view chain. This LayoutView can use an existing element on the page to begin the view tree from. This is better in so many ways. It reduces the complexity of explaining how things work. You no longer need to even think about how a RegionManager – or the code that it represents – even exists. Marionette will become views all the way down. It's a simple system that's so easy to explain, and much easier for new users to understand.

@jmeas I dig the idea of Views all the way down. When creating a child view in a LayoutView, any "region" type logic would be hidden away to the user. So instead of thinking view->region->view, the user would just add a child view via addChildView or whatever, and all tree render/destroy stuff would be abstracted away.

@Anachron
Copy link

That's true, but that is a bad feature. Applications should just be containers. Instead of app.addRegion, we should promote creating a LayoutView as the base of the view chain.
You have my 👍

@rhubarbselleven
Copy link
Contributor

Should it be called LayoutView? Doesn't seem very view-like.

(2) you render the layoutView twice

omg +1

On the pre-render vs existing. From an api point of view, this should be seamless to the dev.

@trezy
Copy link
Member

trezy commented Nov 4, 2014

Is the replaceElement (issue|pull request) stuff going to be a part of this?

@jamesplease
Copy link
Member Author

absolutely.

@trezy
Copy link
Member

trezy commented Nov 5, 2014

SHWEET.

@jasonLaster jasonLaster modified the milestones: v3.0.0, v3.x Aug 18, 2015
@paulfalgout
Copy link
Member

paulfalgout commented May 8, 2017

@marionettejs/marionette-core
So I've been playing with what Region might look like in the future. I think the biggest win is designing the region more around being given an el instead of being passed a selector.
Because we have a getRegion interface that pre-renders the view we can change it such that a region isn't created until it is requested, and that it is created with an existing el from the view.

Then when the layout view is re-rendered rather that destroying and recreating a region we can just give the region a new el to attach it's view to. At first I also supported recreating regions, but this potentially breaks listeners.. If you've gotten a region and then re-render the view, the region you previously got would be leaky and not exist on the view.

  • Rather we should only ever instantiate a singular region per name/selector for a view's lifecycle.
  • It should only be instantiated once requested
  • When a view re-renders the view should give its region a new el, or if there is no longer a valid el rendered, empty the region... maybe destroy the region at this point?

I think the code to achieve this would look like:

  constructor(options) {
    this._setOptions(options);
    this.mergeOptions(options, ClassOptions);
    this._setElement(this.el);
    MarionetteObject.call(this, options);
  },

  setElement(el) {
    const view = this.currentView;

    if (!view) {
      this._setElement(el);
      return this;
    }

    const replaceElement = this._isReplaced;
    this._detachView(view);
    this._setElement(el);
    this._attachView(view, { replaceElement });
    return this;
  },

  _setElement(el) {
    this.$el = this.getEl(el);
    if (this.$el.length !== 1) {
      throw new MarionetteError({
        name: 'BadElError',
        message: `An "el" must be a single DOM node, but ${ this.$el.length } nodes were found. cid: ${ this.cid }.`
      });
    }
    this.el = this.$el[0];
  },

By default setElement would be non-destructive for the child views.. so in order to keep the current behavior we'll have to do something like shouldDestroyView: true:

render() {
  ...
  // this._reInitRegions()
  _.each(this._regions, region => {
    if (region.shouldDestroyView()) region.empty();
    const regionEl = this.$(this.regions[region.name]);  // removes the need for `parentEl`
    if(regionEl) region.setElement(regionEl);  // something like this-ish
    else region.destroy();
  });
}

setElement would essentially replace region.reset()

@blikblum
Copy link
Collaborator

blikblum commented May 8, 2017

So I've been playing with what Region might look like in the future. I think the biggest win is designing the region more around being given an el instead of being passed a selector.

We will still be able to declare region like:

regions: {
   myRegion: '.myclass'
}

or will have to pass a DOM el or jQuery?

By default setElement would be non-destructive for the child views..

what happens with replaceElement ?

@paulfalgout
Copy link
Member

paulfalgout commented May 8, 2017

Yep regions would be declared identically. Almost everything could be an implementation detail for the view, no need to change, except it's breaking because the Region API is also public, but the region's mixin that the view uses would query and cache the selector before sending it to the region. Currently the cost of querying is offset until the first show inside the region, but because we can wait until the first getRegion we can query on the view. This also solves the awkwardness of the region's $el not being available until the first show

what happens with replaceElement ?

if it is replaced, the detach will swap the el.. then the el will get changed and if it was previously replaced it would replace the element on attach. So setElement would handle the replace state at the time of calling the set

@blikblum
Copy link
Collaborator

blikblum commented May 8, 2017

@paulfalgout Thanks. I think is good direction

@JSteunou
Copy link
Contributor

JSteunou commented May 9, 2017

👍

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

9 participants