Skip to content

Allow the confirmation dialog to be optional & add callback_id param#64

Open
benperove wants to merge 4 commits intomaknz:masterfrom
benperove:master
Open

Allow the confirmation dialog to be optional & add callback_id param#64
benperove wants to merge 4 commits intomaknz:masterfrom
benperove:master

Conversation

@benperove
Copy link
Copy Markdown

I've updated to make button confirmations optional. Slack suggests to use confirmations sparingly:
https://api.slack.com/docs/message-buttons#guidelines_and_best_practices

Also, callback_id is a required attachment field:
https://api.slack.com/docs/message-buttons#attachment_fields

@craigwillis85
Copy link
Copy Markdown

craigwillis85 commented Aug 19, 2016

@maknz, @benperove - can this please be merged in? I'm wanting to use the Button API, but it needs the callback_id and it is currently missing from this and the PR fixes it for me

@benperove - are you able to fix the failing tests and update the PR?

@maknz - if the above is done, can you please merge?

@maknz
Copy link
Copy Markdown
Owner

maknz commented Aug 20, 2016

Happy to merge if the test suite passes. Unfortuantely I'm low on time so I won't be able to do the necessary work to make a stable release, at least right now.

@benperove
Copy link
Copy Markdown
Author

The code that I added was pretty straightforward, but I was unaware the tests (or lack thereof) were going to cause the build to fail, so my apologies if I've held anyone up because of this. I will try to add the unit tests possibly by this weekend and get everything squared away.

@funkytaco
Copy link
Copy Markdown

Hi, I know this isn't ready yet, but how do I put a callback_id in my client? Can I just put it in my $slackSettings?

Here is my config.

$slackSettings = [
    'username' => 'foo',
    'link_names' => true
];

$injector->define('Main\Modules\Slack\Slack_Client_Module', [
      ':endpoint' => $arrCfgEnvSettings['slack']['foo']['webhook-url'],
      ':attributes' => $slackSettings,
      ':guzzle' => null
 ]);

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.

4 participants