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

Adds 'component' option to notification options #185

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

Conversation

joeyespo
Copy link

@joeyespo joeyespo commented Aug 25, 2017

I found @schickm's solution very useful for, e.g. adding a link inside the notification. So here's a PR using his example and rebased onto master.

Thanks!

Copy link

@sdhull sdhull left a comment

Choose a reason for hiding this comment

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

Seems like a good feature to have 👍

@bendemboski
Copy link

I'd love to switch to this addon from my home-grown code that implements similar functionality, but being able to fire actions from links in the notification is a critical feature for me, and as far as I can tell there's no way to do that without this PR being merged. Anything blocking it, or any expectation on if/when it might be merged and released? Anything I can do to help it along?

@mansona
Copy link
Owner

mansona commented Apr 19, 2018

Howdy folks 👋 I'm going to be diving into these sorts of PRs over the coming weeks so I'll see what I can do to make a feature like this land.

I will say that at first glance I'm not sure I'm too happy to continually expand the API like this without a bit more structure 🤔

As it was pointed out in this PR #202 there is starting to be a mismatch between how config items are set on an individual component level and how the defaults are configured. I don't want to expand the API until we can find a better design for this overall.

Also we're going to be focusing on testing a little bit more in the coming weeks, both from a perspective of making sure the features of this addon work in the first place but also allowing better ergonomics for people who want to test that notifications are being raised in the consuming app's tests.

@billdami
Copy link

Any movement on this? Being able to define notification content in handlebars, mainly to allow for binding of actions to buttons and whatnot would be huge.

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.

5 participants