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

Make bindings extensible #59

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RickButler
Copy link

@RickButler RickButler commented Feb 24, 2018

For some background on the issue #53 and AmpersandJS/ampersand-view#178

This should also satisfy #16 and #37

The binding store is a singleton loosely modeled after epoxy. It allows adding custom bindings to Ampersand without overwriting the default bindings and is much more powerful than a custom binding function. I did not change any of the logic inside the default-bindings.

Concerns and Notes: I moved several reusable functions into utils (following what seemed to be the pattern from ampersand-events). I would like to move setAttrbiutes and removeAttributes in ampersand-dom to clean things up. I would then move get-matches into the root as get-matches.js.

Originally I had the ampersand prefix on binding-store and default-bindings, I removed them because it made the file name too long and repetitive if you are accessing them via require e.g.
require('ampersand-dom-bindings/ampersand-binding-store')
becomes
require('ampersand-dom-bindings/binding-store')

@RickButler
Copy link
Author

Tagging @cdaringe because of your involvement in the original issue, and @dhritzkiv since you are the most active maintainer.

@dhritzkiv
Copy link
Member

Hey @RickButler. Thanks for this comprehensive PR. I think this looks super promising and useful, especially after a little cleanup.

However, seeing as this is a large and fundamental change, I don't feel comfortable reviewing this alone. I'd need commitment from another team member before starting to discuss this further.

@RickButler
Copy link
Author

Thanks @dhritzkiv, if you see any style changes, bug, errors, etc. Let me know.

Should I tag anyone else?

@dhritzkiv
Copy link
Member

No, I think we're the only two active-ish maintainers, unfortunately. I've added us as reviewers for the PR

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.

None yet

2 participants