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

Attachment support #3

Merged
merged 8 commits into from
Feb 21, 2020
Merged

Attachment support #3

merged 8 commits into from
Feb 21, 2020

Conversation

Funfare
Copy link
Collaborator

@Funfare Funfare commented Feb 21, 2020

Hi,

I added support for attachments and more options to a message (alias, avatar, emoji) and changed the readme.

Think you can close the PR in the old repository.

namespace NotificationChannels\RocketChat;

final class RocketChatMessage
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would not recommend final. We don't know what the user does, I see no problem with allowing to inherit from that class

Copy link
Collaborator

@antonkomarev antonkomarev Feb 21, 2020

Choose a reason for hiding this comment

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

If it will be a problem - user will open an issue and describe a problem. We can remove final and it wouldn't be a breaking change. But you cannot do opposite thing without breaking changes.

@Funfare Funfare merged commit d6e3c81 into laravel-notification-channels:master Feb 21, 2020
@antonkomarev
Copy link
Collaborator

antonkomarev commented Feb 21, 2020

@Funfare squash & merge PRs with more than 1 commit, please. It's much easier to track down commit log when you are working this way.

*
* @return $this
*/
public function clearAttachments(): self
Copy link
Collaborator

@antonkomarev antonkomarev Feb 22, 2020

Choose a reason for hiding this comment

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

Why do we need this method?
Opened discussion in #8

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.

2 participants