Skip to content

Conversation

@fabiopelosin
Copy link

See #11.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@sarahhodne
Copy link
Contributor

Looks good, I'm just curious about the notification activation change.

There are some things I want to change, like refactoring the part in -[AppDelegate shouldShowNotificationFor:] into a filter, but I'll base those changes on your commit. This should then be marked as merged once I push up those changes.

@fabiopelosin
Copy link
Author

I guessed that you would have wanted it as a filter. However it is not clear to me if you intend to use a single filter or combine multiple filters to compute whether the notification should be displayed, and thus I went for the basic approach 😄.

@sarahhodne
Copy link
Contributor

I need to add a different kind of filter – a BuildFilter – and then a way to add combinations of filters. I have it mostly planned out already, I was just waiting to implement it.

Don't worry about the fact that the pull request can't be merged automatically—I have the conflict resolved locally.

@fabiopelosin
Copy link
Author

Don't worry about the fact that the pull request can't be merged automatically—I have the conflict resolved locally.

Cool to hear that. I didn't noticed the conflict, and I'm not sure how I generated it, I thought I started from the tip of master.


Btw, a nit pick: it would be great if the T of the menu bar icon was gray or transparent. Imo the white fill pops out a little too much.

@sarahhodne
Copy link
Contributor

Cool to hear that. I didn't noticed the conflict, and I'm not sure how I generated it, I thought I started from the tip of master.

You did, I just had some local commits that I forgot to push.

@sarahhodne sarahhodne removed their assignment Mar 20, 2014
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