Skip to content
This repository has been archived by the owner on Jun 4, 2018. It is now read-only.

Added ability to add actions (buttons) within the attachment #32

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added ability to add actions (buttons) within the attachment #32

wants to merge 3 commits into from

Conversation

craigwillis85
Copy link

Hi

I have added a little bit of code to make it so that actions can be added within an attachment. Feel free to change if it doesn't conform

*/
public function __construct($title, $text, $fallback = null, $color = null, $pretext = null, array $fields = [])
public function __construct($title, $text, $fallback = null, $callback_id, $color = null, $pretext = null, array $fields = [], array $actions = [])
Copy link

Choose a reason for hiding this comment

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

One thought here, you probably want to use $callback_id = null in the constructor here, otherwise it breaks the existing API contract for the Attachment object and will break code on upgrade.

If you really meant to require the field, you probably should move it before $fallback as this order would require all constructor calls to also specify a value, or null, for $fallback as well.

Copy link

@jshayes jshayes Oct 18, 2016

Choose a reason for hiding this comment

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

If it is changed to $callback_id = null then it should also be moved to the end of the constructor, after all the existing parameters, otherwise it will still be a breaking change.

Copy link

Choose a reason for hiding this comment

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

Yeah, I was having a small brain-fart on that positioning... 😨

@craigwillis85
Copy link
Author

@bdaroz I've updated the code as mentioned in the comments

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants