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.

Sign up for GitHub

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

Jump to bottom

Adds onAttach triggerMethod. #1957

Closed
wants to merge 28 commits into from
Closed

Adds onAttach triggerMethod. #1957

wants to merge 28 commits into from

Conversation

jamesplease
Copy link
Member

Adds triggerAttach and beforeTriggerAttach triggerMethods.

This is the 3rd out of 3 PRs to get #1886 in.

@jamesplease jamesplease force-pushed the trigger-attach branch 6 times, most recently from 636459e to 83c144a Compare October 2, 2014 18:57
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 83c144a on jmeas:trigger-attach into 42eb37a on marionettejs:minor.

paulovieira
paulovieira reviewed Oct 3, 2014
View reviewed changes
src/region.js

if (attachedRegion && this.triggerBeforeAttach) {
displayedViews = [view].concat(_.result(view, '_getNestedViews'));
this._triggerAttach(displayedViews, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a boolean flag in _triggerAttach, we could pass the prefix string directly:

this._triggerAttach(displayedViews, 'before:');
this._triggerAttach(displayedViews, '');

This would make the _triggerAttach role more clear, even without looking at it.

@paulovieira
Copy link
Contributor

A couple notes:

triggerBeforeAttach and triggerAttach can only be defined in the prototype. I think it makes sense to allow to provide (override) these options in the .show method, along with preventDestroy and forceShow.

The use case is when the user wants triggerBeforeAttach and triggerAttachalways false (for performance reasons), but for a particular view the attach event has to be triggered. We should provide fine grained options in that case.

myRegion.show(myViewWithSomePlugin, { triggerAttach: true})

Should we add "before:detach" / "detach" events as well? These might be necessary to shut down 3rd party plugins.

In principle it could (and should) be done on the view's "destroy". But if preventDestroy is true that event will not be fired.

@jamesplease
Copy link
Member Author

  1. Agreed. Will add.

  2. Maybe. That discussion should be had in a separate issue, though. Feel free to make one.

@paulovieira paulovieira mentioned this pull request Oct 4, 2014
Closed
@jamesplease
Copy link
Member Author

Updated with @paulovieira's comments, but I need to do three things

  1. Test passing options into the show method
  2. Ensure that you can add new views in onBeforeAttach within the tests
  3. Update the docs regarding the new show options

@jamesplease jamesplease force-pushed the trigger-attach branch 2 times, most recently from 8bdc339 to dd358e4 Compare October 4, 2014 02:53
@jamesplease
Copy link
Member Author

Alright. All updated and ready for review again.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling fa8f0a5 on jmeas:trigger-attach into 68953b3 on marionettejs:minor.

@jamesplease
Copy link
Member Author

Oh, note to self...I need to add the right args here.

jasonLaster
jasonLaster reviewed Oct 5, 2014
View reviewed changes
src/region.js
triggerBeforeAttach: true,
triggerAttach: true,

_triggerAttach: function(views, prefix) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm maybe triggerMethodMany or pull the each up. You don't need the if if triggerMethodOn is smart.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this with some clever _.union code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revisiting this function again -

what I'd like to be able to do is write:

if (this.isAttached() && shouldTriggerAttach) {
   this.triggerMany(this._displayedViews(), 'attach', this);
}

So, i realize that this is a lot of changes rolled up in one snippet:

  1. isAttached is discussed above
  2. shouldTriggerAttach is discussed above
  3. _displayedViews is discussed above

triggerMany is a nice abstraction because it cleans up the loop. Also, I don't like triggerAttach because it is attach specific. If we keep it I'd call it triggerMany and pass in the full event name so that it can be re-used. w/r/t passing the view as the first event argument - i think that makes a lot of sense and we can do that with some concat action.

jamesplease and others added 7 commits October 7, 2014 16:15
@jamesplease @samccone
Adds helper methods to AppRouter.
b2cda35
@paulfalgout @samccone
region._ensureViewIsIntact
ebb0e1c 
Having the region check for view.isDestroyed does two things:

1. It allows you to handle the error of a region.show without the region killing the currentView and breaking without recourse.

2. appending isDestroyed to a Backbone.View on region empty adds the same safety for not re-showing a removed backbone view.

The extra return this; was removed to prevent jslint error for too many statements, and seemed unnecessary
@jamesplease @samccone
Improves Application triggerMethod arguments.
9e6bcaf
@jdaudier @samccone
Update param types for addRegion method
cd655ee
@jamesplease @samccone
Removes unnecessary emptyRegions call in LayoutView.
c2fcd3d
@jamesplease @samccone
Updates file names; remove unnecessary files.
9c22308
@jdaudier @samccone
Update region-manager.jsdoc
ed6cec5
@jamesplease jamesplease force-pushed the trigger-attach branch 2 times, most recently from 4670252 to 12f1904 Compare October 8, 2014 03:26
@jamesplease
Copy link
Member Author

Updated @jasonLaster.

  • No more if statement
  • No more testing of the implementation within the tests

I also added some good arguments for the trigger method: the view itself as the first argument, and the region as the second.

This is ready for a final review and some final thumbz. Let's get this in...I still need to add onDetach!

@jamesplease jamesplease added this to the v2.3 milestone Oct 8, 2014
jasonLaster
jasonLaster reviewed Oct 11, 2014
View reviewed changes
src/region.js
@@ -87,8 +87,27 @@ Marionette.Region = Marionette.Object.extend({
this.triggerMethod('swapOut', this.currentView);
}

// An array of views that we're about to display
var attachedRegion = Marionette.isNodeAttached(this.el);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable name suggests that it should reference the "attached region" which is unclear.

I think a better name would be isRegionAttached and i think that could be moved to a function (this.isAttached()).

@jasonLaster
Copy link
Member

Awesome work @jmeas. Added lots of comments, but this guy is good and will get in soon

@jasonLaster
Copy link
Member

@jmeas let's get this guy in. Then onDetatch

@jamesplease jamesplease mentioned this pull request Oct 21, 2014
Closed
@jasonLaster
Copy link
Member

Hey @jmeas, I just rebased master onto minor. mind doing a quick rebase and re-push.

@jasonLaster jasonLaster mentioned this pull request Oct 31, 2014
Closed
@jasonLaster
Copy link
Member

Closing in favor of #2043.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
v2.3
Development

Successfully merging this pull request may close these issues.
8 participants
@jamesplease @coveralls @paulovieira @jasonLaster @paulfalgout @jdaudier @samccone @bendman