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

Add isFirstRender flag during rendering phase #3428

Closed
wants to merge 1 commit into from

Conversation

JSteunou
Copy link
Contributor

@JSteunou JSteunou commented Aug 16, 2017

Call on showChildView to set children views is usually set during onRender, but as regions & children are not always erased on re-rendering (it depends on the Renderer the user set) I think you should offer another way to know more easily if this call should be done or not.

Today we have this check

if (!this.getRegion('foo').hasView()) {
  this.showChildView('foo', new myView());
}

or user can set its own flag

onRender() {
  this._firstRender = this._firstRender === undefined;
  if (this._firstRender) {
    this.showChildView('foo', new myView());
  }
}

I would like Marionette offers the second logic to all Views, because it will be more versatile

This will be a great feature along with #3427

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling dd0f23d on JSteunou:first-render into 1962fbb on marionettejs:next.

@blikblum
Copy link
Collaborator

Today we have this check

if (!this.getRegion('foo').hasView()) {
  this.showChildView('foo', new myView());
}

Currently this does not work in onRender because hasView is always false because the regions are always re initialized

an alternative is to add a method ensureChildView method that would only show the child view if not exists

this.ensureChildView('myregion', () => new MyView({}))

It could be used safely inside onRender

Anyway, any solution would require an option to not reinit the regions

@blikblum
Copy link
Collaborator

Yet another alternative is to make the show child view declarative in the region definition

var MyView = View.extend({
  regions: {
     myregion: {
        el: '.my-region',
        view: function () { return new MyChildView ()},
        showOnlyOnce: true
     }
  }
})

or

var MyView = View.extend({
  regions: {
     myregion: {
        el: '.my-region',
        viewClass: MyChildView,
        viewOptions: function() {return {model: this.model}}, 
        showOnlyOnce: true
     }
  }
})

The advantage is that the implementation of how the child view is show would not be exposed to user. We could change it safely

@JSteunou
Copy link
Contributor Author

Currently this does not work in onRender because hasView is always false because the regions are always re initialized

True, I was already ahead of #3427

I dont know if I like the declarative way with the showOnlyOnce, but if there is a way to make it use ensureChildview under the hood I'm in

@paulfalgout
Copy link
Member

paulfalgout commented Aug 16, 2017

Again I think this further complicates the view

const MyView = Mn.View.extend({
  initialize() {
    if(this.isRendered()) {
      // preexisting DOM
      this.showChildView('view1', new View1());
      this.showChildView('view2', new View2());
    }
  },
  onRender(view, firstRender) {
    // Will only be called if the view is re-rendered
    if(firstRender) { 
      console.log('If view had preexisting DOM I will never occur');
       this.showChildView('view1', new View1());
     }
    if(this.isRendered()) { console.log('If view had preexisting DOM  I will always occur'); }
    this.showChildView('view2', new View2());  // render every time
  }
});

vs

const MyView = Mn.View.extend({
  // Will be called whether or not the view has preexisting el but only once  
  onReady() {
    this.showChildView('view1', new View1());
    this.showChildView('view2', new View2());
  },
  onRender() {
    if(this.isRendered()) {  // will be called every render but only true if !onReady
      this.showChildView('view2', new View2());
    }
  }
});

I would suggest with new renders and non-destructive regions, the onReady with no additional logic will be the place most people would need to instantiate child views... otherwise to answer the question, "where do I create child views?" we first have to answer.. ok are you using pre-existing dom? which renderer are you using? If you're using that renderer are you marking the children you don't want removed? marionette now has non-destructive regions, are you using those? if so your solution is to check these flags in this case.. these flags in another.. sometimes they're on the view.. in some cases they're passed via an event etc etc etc etc etc

@JSteunou
Copy link
Contributor Author

Good point I like it better !

@JSteunou JSteunou closed this Aug 16, 2017
@blikblum
Copy link
Collaborator

// onReady Will be called whether or not the view has preexisting el but only once

onReady must be called after every render with a destructive renderer

@JSteunou
Copy link
Contributor Author

@blikblum I though we could introduced the ready event only for non destructive view at first (this would make the change not breaking) then on a major release make all view non destructive by default. In this major release, if a user want the destructive behavior he could call showChildView at each render.
This would keep ready all its way and make it not double with render

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants