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

Pass card options to card/atom components #122

Closed

Conversation

kylenathan
Copy link
Contributor

Fixes #82

@decasia - Hope you dont mind, we needed this quite urgently so have pretty much copied your fork, added a fix and some tests.

Nudging @mixonic

@kylenathan kylenathan force-pushed the pass-options-to-cards-and-atoms branch from a24ae6a to b451a22 Compare January 9, 2017 21:10
@decasia
Copy link

decasia commented Jan 10, 2017

Don't mind at all, @kylenathan — I apologize to the project maintainers for letting issue #82 languish for so long, and I'd still love to see the functionality get merged!

@@ -217,6 +220,7 @@ export default Component.extend({
this.get('componentAtoms').removeObject(atom);
}
};
editorOptions.cardOptions = assign(componentHooks, editorOptions.cardOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

@kylenathan we're not convinced that merging user cardOptions over the editor built-in hooks is best. Really the built-in hooks should not need to be stomped, and if they are stomped we will end up with a mess.

At some point we should make the ADD_CARD_HOOK constant into a symbol, since we don't want the property name to be guessable by the outside world, and don't want it to conflict either.

This can just be reversed though, and the componentHooks can win. I'm going to make that change and land this 🎉

@mixonic
Copy link
Contributor

mixonic commented Feb 9, 2017

Replaced by #125

@mixonic mixonic closed this Feb 9, 2017
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.

Passing context or metadata into component cards/atoms
3 participants