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

Remove unused listener from base view class. #2262

Merged
merged 1 commit into from
Feb 4, 2015

Conversation

samccone
Copy link
Member

@samccone samccone commented Feb 4, 2015

because good call @jmeas in #2133

@samccone samccone self-assigned this Feb 4, 2015
@samccone samccone added this to the v2.4 milestone Feb 4, 2015
@samccone samccone added the minor label Feb 4, 2015
@samccone
Copy link
Member Author

samccone commented Feb 4, 2015

yeah it triggers show on all childviews when the parent is shown

@jamiebuilds
Copy link
Member

oooooh got it

👍

@paulfalgout
Copy link
Member

Hmm I'd be up for renaming onShowCalled to something else.. not sure what.. _onShow _proxyShowEvent I dunno... at least _onShowCalled
Also onShowCalled should be removed from view https://github.com/marionettejs/backbone.marionette/blob/minor/src/view.js#L138

This is breaking, kind of, since it doesn't follow naming conventions.. I don't think this functionality is documented, but someone somewhere could be doing this:

var MyView = Marionette.ItemView.extend({
  onShow: function() {
    this.doSomething();
  },
  onShowCalled: function() {
    alert('Why God why?!?');
  }
});

@samccone
Copy link
Member Author

samccone commented Feb 4, 2015

yeah we say it is internal
https://github.com/marionettejs/backbone.marionette/blob/minor/src/view.js#L137

good catch... deleting

also @paulfalgout i am all for renaming the method

@paulfalgout
Copy link
Member

Ah! Ha. Missed the inline comment :-)

@samccone samccone force-pushed the sjs/fix-unused-listener branch from 04fa350 to 46af8a3 Compare February 4, 2015 02:57
@paulfalgout
Copy link
Member

👍

@jamesplease
Copy link
Member

It should describe what it's doing, so I'm in favor with a name like _propagateShowEvent.

@jamesplease
Copy link
Member

Also, I could actually see regular old LayoutViews propagating this event to their children views, too. This will be of particular importance after #1263.

@samccone
Copy link
Member Author

samccone commented Feb 4, 2015

yeah i could see that also @jmeas this is mostly just a minor branch fixup

@jamesplease
Copy link
Member

👍

1 similar comment
@jamiebuilds
Copy link
Member

👍

jamiebuilds added a commit that referenced this pull request Feb 4, 2015
Remove unused listener from base view class.
@jamiebuilds jamiebuilds merged commit 7e47ba5 into minor Feb 4, 2015
@jamiebuilds jamiebuilds deleted the sjs/fix-unused-listener branch February 4, 2015 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants