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

Add collapsible open/close events #358

Merged

Conversation

drub0y
Copy link
Contributor

@drub0y drub0y commented Jan 17, 2017

Main work here is surfacing the open/close events from the Materialize collapsible component in friendly ways for the Aurelia developer. The developer can hook these events in one of two ways:

  1. Using the .call expression in the attribute to directly bind a function call on their view model via the new onOpen and onClose properties (note: on-open and on-close in attribute syntax!).
  2. Via the EventAggregator infrastructure where two new events are published in the form of: aurelia-md-bridge:collapsible:[open|close]. I would especially like feedback on whether or not you guys want the framework to be publishing via EventAggregator and, if so, if you "like" the naming scheme I've chosen.

NOTE: this also adds a new sample for the Collapsible component from this gist to demonstrate how both approaches could be used to the caller. If you think these should be broken up into two different samples I can certainly do that.

Ancillary work included here just updates some project level settings to make the project more friendly to work with (at least from VSCode). Details in commit message for 669e8db should be pretty straightfwd so I won't repeat them here.

@Thanood
Copy link
Collaborator

Thanood commented Jan 17, 2017

Thanks a lot, @drub0y! 👍

I'm a little unsure about the event aggregator. When users subscribe to onClose/onOpen they could fire their own global events from there, right?

@drub0y
Copy link
Contributor Author

drub0y commented Jan 17, 2017

Yes, they could. It's really just a question of whether the component itself offers multi-cast style eventing via the EventAggregator or not. If nothing else in the bridge is doing this (or planning on adding it) then, I agree, it doesn't make sense for this one component to do it. If anything you'd want to offer events throughout the entire framework for a consistent development experience.

@drub0y
Copy link
Contributor Author

drub0y commented Jan 17, 2017

I ripped out the EventAggregator support, simplified the implementation and sample.

@Thanood
Copy link
Collaborator

Thanood commented Jan 17, 2017

Thanks a lot!
I really like the sample, too. 😃

Maybe I should have told you before.. We have some commit message conventions which simplify changelog generation.. Would you mind squashing commits and change the commit message to (for instance) "feat(md-collapsible): add onOpen/Close event support"?

Sorry for mentioning it so late. 😃
If you want, we can skip it this time and I'll change it after merging, no problem.

 * Add onOpen/Close event support to collapsible (Fixes aurelia-ui-toolkits#348)
 * Some additional project level setting updates to make project friendlier to work with:
   * Disable jshint for VSCode since eslint is being used
   * Exclude additional, project specific directories from JS tooling
@drub0y drub0y force-pushed the add-collapsible-openclose-events branch from 51fbe47 to a1f1e61 Compare January 18, 2017 22:30
@drub0y
Copy link
Contributor Author

drub0y commented Jan 18, 2017

Done!

@Thanood
Copy link
Collaborator

Thanood commented Jan 18, 2017

Thank you, sir! 😃

@Thanood Thanood merged commit ee37ef8 into aurelia-ui-toolkits:master Jan 18, 2017
@Thanood
Copy link
Collaborator

Thanood commented Jan 19, 2017

Released and published in 0.22.0.

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.

2 participants