adopted-ember-addons / ember-cli-flash

Simple, highly configurable flash messages for ember-cli
https://www.npmjs.com/package/ember-cli-flash
MIT License
355 stars 113 forks source link

Pass 'close' as closure action #230

Closed caseywatts closed 7 years ago

caseywatts commented 7 years ago

This adds in the ability to close the flash message in your own custom components. I tweaked the example in the readme to include a close button in the template there. I've also tried this in my own app :)

If we like this idea, we should probably make a separate example that has close on click set to false (since currently this example it wouldn't quite make sense why we want the close button when you can click on the whole thing to close it).

caseywatts commented 7 years ago

I'm not 100% sure I want to yield it as another ordered parameter, but I don't quite wanna make a separate actions hash for it either lol For this concern, I'm mostly worried about future edits to this addon - it works great as-is, no problem ✨

caseywatts commented 7 years ago

I tried writing some tests for this feature, but they all suck so far lol: https://github.com/poteto/ember-cli-flash/pull/231

caseywatts commented 7 years ago

This branch has an integration test now - this PR should be good to go! :)

caseywatts commented 7 years ago

@sbatson5 could you look over this when you get a chance? :)

sbatson5 commented 7 years ago

@caseywatts sorry for the delay. The test looks good. Just want to pull it into a dummy app real quick and test it. Should be able to get to that tonight

caseywatts commented 7 years ago

sweet! thanks @sbatson5 :)

caseywatts commented 7 years ago

I imagine this is a pretty common use case and I'd love to make it super straightforward, too.

--

This is a pretty common fork-in-the-road when designing Ember components. I'm not 100% that my suggestion is the better of the two paths, but I can provide more arguments for it at least :)

contextual-component vs pass-in-more-arguments

@sbatson5 we're also going to want to put some classes on this - that'll lead us to three arguments. Next (and this is probably going too far lol): some people may want this to be a styled button. If we wanted all of these, the API for this becomes something like this:

get(this, 'flashMessages').success('Success!!', {
  flashAction: {
    action: this.get('close'),
    label: 'Close',
    classNames: 'xyz',
    tagName: 'button'
  }
});

I think a contextual component sort of thing gets around this sort of argument-passing-bloat pretty nicely :)

Also - the last argument close is optional to write, so I don't believe it'll break any existing code or examples. It should be a ~backwards-compatible change.

sbatson5 commented 7 years ago

@caseywatts ok, you convinced me :). Would you mind just updating the README and just mention to disable destroyOnClick if you want to use the close action? Once that's in, we're good to merge 👍 (sorry for the delay)

caseywatts commented 7 years ago

updated the readme! 🎉

caseywatts commented 7 years ago

typo corrected ✨

caseywatts commented 7 years ago

and it's in! 🎉

now that master displays this option, we'll probably want to do a release soon so it matches up? cc @sbatson5 @Dhaulagiri

sbatson5 commented 7 years ago

Unfortunately, I don't have npm access to this package, so we will need @Dhaulagiri to publish and update for us.

Dhaulagiri commented 7 years ago

published in 1.4.3 🎉