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

Update Marionette functions #927

Closed
jamesplease opened this issue Feb 20, 2014 · 8 comments
Closed

Update Marionette functions #927

jamesplease opened this issue Feb 20, 2014 · 8 comments

Comments

@jamesplease
Copy link
Member

There are five Marionette helper functions, and they behave in two different ways. The first type are attached to prototypes, and called directly via this.someFn. The other type are left on Marionette, and are called with the target as the first argument: Marionette.someFn( this, args... ).

It seems the decision was made in PRs like this one to stop passing the target as the first option, and attach the methods directly to the prototypes.

These are the functions and whether or not they're attached to prototypes or not:

✓ normalizeMethods
✓ triggerMethod
✓ extend
getOption (to-be removed in v2)
✗ bindEntityEvents

If we want to change getOption and bindEntity to be consistent with the other methods I can make up a PR 👍

@dminkovsky
Copy link
Contributor

This would be lovely. I'm using getOption rarely just because it takes up so much space. I usually just do this.option because I know it'll be there.

@jasonLaster
Copy link
Member

Nice, would love to see this go in!

@samccone samccone added this to the v2.0.0 milestone Feb 28, 2014
@cobbweb
Copy link
Member

cobbweb commented Mar 3, 2014

👍 This reminds me that we need to sort out the filenames, some are camelCased while others are just lowercase.

@fenduru
Copy link

fenduru commented Mar 12, 2014

I would absolutely love this. I was about to fork and add this in, but checked for discussion first and found this. @jmeas do you plan on making a pull request soon, or should I go ahead and do so?

@jamesplease
Copy link
Member Author

@fenduru if you'd like to make the PR go ahead. v2 is still a bit of a ways off, but I'd appreciate the help!

@samccone
Copy link
Member

maybe not so far off 👍

@fenduru
Copy link

fenduru commented Mar 12, 2014

I missed the v2 tag in the title. Since this change would not be breaking, could it also be added to master?

@jmeas Sounds good. Will hopefully make the PR later today

@jamesplease
Copy link
Member Author

Fixed in v2 by #1156

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

6 participants