Pass 'close' as closure action

[caseywatts]

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]

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]

I tried writing some tests for this feature, but they all suck so far lol:
#231

[caseywatts]

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

[caseywatts]

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

[sbatson5]

@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]

sweet! thanks @sbatson5 :)

[sbatson5]

I'm wondering if we can expand this to be a bit more flexible. Rather than adding a closure action to the component, we could add a flashAction property to the flash object itself. This keeps us from yielding another property and would make it easier to control the destroyOnClick logic. We could also have the label of the action button as another property. So we could create messages like:

get(this, 'flashMessages').success('Success!!', {
  flashAction: {
    label: 'Close'
  }
});

Then we could add a default action property that calls flash.destroyMessage(). Or the user could overwrite it with their own action if they wanted to do something other than closing (i.e. they wanted to rollback attributes on a model or something similar).

Although this adds some extra conditions to the template:

{{#flash-message flash=flash}}
    {{flash.message}}
    {{#if flash.flashAction}}
      <a href="#" {{action (action flash.flashAction.action)}}>
        {{flash.flashAction.label}}
      </a>
    {{/if}}
  {{/flash-message}}

So if the user sets a flashAction, we render it with their label and default flashAction.action to destroyMessage, otherwise we use an action they pass in.

This may be overcomplicating the API, however. Let me know your thoughts!
cc @poteto

[sbatson5]

For consistency, we try to use double quotes on templates. Can you wrap close in double quotes?

[sbatson5]

🤔 I'm wondering if there is a way for us to default this to false when yielding/using the close action. It could be confusing if both are enabled.

[caseywatts]

uh oh right, destroyOnClick=true is the default :/
I was surprised that was true when I first read the docs for this. And I'm surprised again just now haha (I wonder if other users are surprised by this default too?)

Hmm - we could at least document it in the README.md next to an example that uses close?

[caseywatts]

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]

@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)

[sbatson5]

What do you think of adding a warning via Ember.Logger.warn if flash.destroyOnClick is true? Something like:

close() {
  if (get(this, 'flash.destroyOnClick')) {
    Logger.warn('This flash message has `destroyOnClick` enabled -- be sure to disable it when using the close action')
  }
  this._destroyFlashMessage();
}

[bcardarella]

@sbatson5 keep in mind that warn is stripped from Production builds: https://emberjs.com/api/#method_warn

[caseywatts]

I like the idea of this!
If it's stripped when this package is packaged (production build) then this one would be stripped out, hrm :/

How do other addons add warnings for users of the addon?

[caseywatts]

updated the readme! 🎉

[sbatson5]

Just 1 small typo. Otherwise, looks good.

[sbatson5]

such *as* an x.

[caseywatts]

typo corrected ✨

[caseywatts]

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]

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

[Dhaulagiri]

published in 1.4.3 🎉