search

travis-ci / travis-watcher-macosx

[DEPRECATED] A Travis CI client for Mac OS X.
MIT License
95 stars 9 forks source link

[Feature] Added failure only notifications #13

Open fabiopelosin opened 11 years ago

fabiopelosin commented 11 years ago

See #11.

sarahhodne commented 11 years ago

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 commented 11 years ago

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 :smile:.

sarahhodne commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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.