laravel / slack-notification-channel

Slack Notification Channel for laravel.
https://laravel.com/docs/notifications#slack-notifications
MIT License
864 stars 59 forks source link

Added ability to set a custom embed color #58

Closed N3rdP1um23 closed 1 year ago

N3rdP1um23 commented 1 year ago

The following PR allows anyone to set a custom embed sidebar color overriding the set level (info, success, warning, error).

In order to use the custom color (using the same snippet from the docs), the developer would have to call customColor function passing in a hex color code.

/**
 * Get the Slack representation of the notification.
 *
 * @param  mixed  $notifiable
 * @return \Illuminate\Notifications\Messages\SlackMessage
 */
public function toSlack($notifiable)
{
    $url = url('/exceptions/'.$this->exception->id);

    return (new SlackMessage)
                ->customColor('#FF2D20')
                ->content('Whoops! Something went wrong.')
                ->attachment(function ($attachment) use ($url) {
                    $attachment->title('Exception: File Not Found', $url)
                               ->content('File [background.jpg] was not found.');
                });
}

This ability allows developers to customize the embed sidebar color outside of the standard levels to possibly correlate with branding or additional status colors.

taylorotwell commented 1 year ago

Does this only apply when using attachments? Would the color be used even if your SlackMessage only called content?

N3rdP1um23 commented 1 year ago

Correct, it only applies when using attachments. Similar to when you're attempting to use success(), error(), warning(), or info() (which I've noticed doesn't actually set a color - as seen in the color function). The color is only applied when using attachments. Calling content alone doesn't set anything unfortunately.

Here's a small comparison table just for reference

Color Attachment/Content Code Slack Output
None Content ```php public function toSlack($notifiable) { return (new SlackMessage) ->content('Testing no color'); } ``` ![image](https://user-images.githubusercontent.com/8093806/208469740-25327297-8725-4e66-8fcd-5fb738ac8b6f.png)
Success (via `success()`) Content ```php public function toSlack($notifiable) { return (new SlackMessage) ->success() ->content('Testing success color'); } ``` ![image](https://user-images.githubusercontent.com/8093806/208469426-0312936c-6115-4352-9262-b4b04d3acc5d.png)
Laravel Red (via `customColor('#FF2D20')`) Content ```php public function toSlack($notifiable) { return (new SlackMessage) ->customColor('#FF2D20') ->content('Testing Laravel red (#FF2D20) color'); } ``` ![image](https://user-images.githubusercontent.com/8093806/208470289-59e54fc0-72f8-456b-85e5-ebb3f7e2d4a0.png)
None Attachment ```php public function toSlack($notifiable) { return (new SlackMessage) ->content('Content Message') ->attachment(function ($attachment) { $attachment->title('Attachment') ->fields([ 'Color' => 'None' ]); }); } ``` ![image](https://user-images.githubusercontent.com/8093806/208472955-48d85efc-7b8c-49ef-95f8-4b938952da41.png)
Success (via `success()`) Attachment ```php public function toSlack($notifiable) { return (new SlackMessage) ->success() ->content('Content Message') ->attachment(function ($attachment) { $attachment->title('Attachment') ->fields([ 'Color' => 'success()' ]); }); } ``` ![image](https://user-images.githubusercontent.com/8093806/208473246-a92e7850-41c2-4591-b99a-66836ef73bf0.png)
Laravel Red (via `customColor('#FF2D20')`) Attachment ```php public function toSlack($notifiable) { return (new SlackMessage) ->customColor('#FF2D20') ->content('Content Message') ->attachment(function ($attachment) { $attachment->title('Attachment') ->fields([ 'Color' => '#FF2D20' ]); }); } ``` ![image](https://user-images.githubusercontent.com/8093806/208473443-8cbe1f5d-88d4-4a28-ae39-50693ea704a7.png)
taylorotwell commented 1 year ago

OK - the reason I ask is if it just pertains to attachments, should it be a method on the attachment object that is passed to the attachment closure instead?

N3rdP1um23 commented 1 year ago

Now that you mention that, yeah that's a much better implementation. It also allows setting it at the individual attachment level rather than for all attachments in the webhook.

Should success(), error(), warning(), and info(), be moved as well as the custom color? Or left alone and only implement the custom color on the attachments object? I can look at updating the PR either way, just wanted to be sure beforehand.

I haven't come across a scenario where they affect when sending just content (as seen in the above images).

taylorotwell commented 1 year ago

I would not move any other methods.

taylorotwell commented 1 year ago

Please mark as ready for review when the requested changes have been made.

N3rdP1um23 commented 1 year ago

Oh man, now I feel silly haha Looking at it more, custom colors have already been implemented at the individual attachment level. Completely missed that looking through the repo the first time.

https://github.com/laravel/slack-notification-channel/blob/8b710d5645ba486106def409c9d114caff2b649a/src/Messages/SlackAttachment.php#L46-L51

https://github.com/laravel/slack-notification-channel/blob/8b710d5645ba486106def409c9d114caff2b649a/src/Messages/SlackAttachment.php#L191-L202

https://github.com/laravel/slack-notification-channel/blob/8b710d5645ba486106def409c9d114caff2b649a/src/Channels/SlackWebhookChannel.php#L90

My apologies for any inconvenience. I'll close off the PR now. Thanks again! 🙂