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

implemented sending of notifications based on segments (included/excluded) #72

Conversation

rylxes
Copy link

@rylxes rylxes commented Jul 4, 2018

ability of users to send notifications excluding a segment i.e

public function routeNotificationForOneSignal() { return ['excluded_segments' => ['excluded segments']]; }

ability of users to send notifications including a segment i.e

public function routeNotificationForOneSignal() { return ['included_segments' => ['included segments']]; }

@LKaemmerling
Copy link
Collaborator

Could you please rebase this on the refactor-to-traits branch?

@rylxes
Copy link
Author

rylxes commented Jul 5, 2018

I have done that, ... the current branch is up to date

@LKaemmerling LKaemmerling changed the base branch from master to refactor-to-traits July 5, 2018 12:42
@LKaemmerling
Copy link
Collaborator

For me this looks okay @Lloople what do you think?

@LKaemmerling LKaemmerling requested a review from Lloople August 8, 2018 07:53
Copy link
Collaborator

@Lloople Lloople left a comment

Choose a reason for hiding this comment

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

It looks good to me. I personally don't like to align array values when multiline but I'm okay with it and it's totally aesthetic. Both method for include or exclude segments looks great 👍

@LKaemmerling LKaemmerling merged commit 32521dd into laravel-notification-channels:refactor-to-traits Aug 16, 2018
@rylxes rylxes deleted the add_segments branch August 18, 2018 11:54
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.

3 participants