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

Add new 'appearance' optional block, constructors to enable proper syncing of sound/vibration with queued notifications. #94

Merged
merged 5 commits into from Aug 29, 2014

Conversation

andyzei
Copy link
Contributor

@andyzei andyzei commented Aug 23, 2014

CRToast automatically handles notification queuing, which is great -- it's essentially fire-and-forget. Currently, there is an optional block that you can provide to be called after a notification shows. Unfortunately, there isn't a way to get notified when a notification is actually being shown.

The scenario here is that several notifications are queued to show, and you want to play a sound / vibration each time a notification is dequeued/shown.

This change simply adds an additional block property to CRToast, and a new constructor that lets the caller provide it. The block is called immediately before the notification is shown.

I did my best to emulate the project formatting / conventions -- if you have feedback, I'm happy to do an iteration or two. Thanks for providing this library -- it's really great.

Thanks,

Andy

Andy Zeigler added 3 commits August 18, 2014 10:56
…efore the notification is actually shown. Useful for synchronizing audio / vibration when multiple notifications are queued.
@Ashton-W
Copy link
Collaborator

Great work Andy. Thanks.

@cruffenach
Copy link
Owner

This is great but I don't think showBegin is a very good name. I was thinking appearanceBlock? Also I would want the signature to be

+ (void)showNotificationWithOptions:(NSDictionary*)options appearanceBlock:(void (^)(void))appearanceBlock completionBlock:(void (^)(void))completion;

rather than

+ (void)showNotificationWithOptions:(NSDictionary*)options completionBlock:(void (^)(void))completion showBeginBlock:(void (^)(void))showBegin;

Thoughts?

@andyzei
Copy link
Contributor Author

andyzei commented Aug 27, 2014

@cruffenach -- great feedback -- agreed completely. Lemme spin up a change....

…I, reorder parameters to reduce dissonance.
@andyzei andyzei changed the title Add new 'showBegin' optional block, constructors to enable proper syncing of sound/vibration with queued notifications. Add new 'appearance' optional block, constructors to enable proper syncing of sound/vibration with queued notifications. Aug 27, 2014
@andyzei
Copy link
Contributor Author

andyzei commented Aug 27, 2014

@cruffenach, all fixed -- lemme know if you have any additional feedback / concerns -- thanks again for taking a look at this!

Andy

@@ -1344,6 +1358,7 @@ - (void)addNotification:(CRToast*)notification {
}

- (void)displayNotification:(CRToast*)notification {
notification.appearance();
Copy link
Owner

Choose a reason for hiding this comment

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

Should be if (notification.appearance) notification.appearance();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed -- will fix -- sorry I missed this

@Ashton-W
Copy link
Collaborator

I wonder if appearance is confusing with UIAppearance :)

@cruffenach
Copy link
Owner

I hear you @Ashton-W, don't think its too big a deal myself. Other suggestions?

@Ashton-W
Copy link
Collaborator

Nah, looks great

@andyzei
Copy link
Contributor Author

andyzei commented Aug 28, 2014

woot

@cruffenach
Copy link
Owner

Can we get this resolved?

@andyzei
Copy link
Contributor Author

andyzei commented Aug 29, 2014

@cruffenach -- wow can't belive I missed that -- thanks. Fixed

cruffenach added a commit that referenced this pull request Aug 29, 2014
Add new 'appearance' optional block, constructors to enable proper syncing of sound/vibration with queued notifications.
@cruffenach cruffenach merged commit 882474d into cruffenach:master Aug 29, 2014
@andyzei
Copy link
Contributor Author

andyzei commented Aug 29, 2014

You guys rock -- thank you so much!

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.

None yet

3 participants