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 terminal-notifier in order to notify when synced #63

Merged
merged 2 commits into from
Jul 21, 2016
Merged

Add terminal-notifier in order to notify when synced #63

merged 2 commits into from
Jul 21, 2016

Conversation

midN
Copy link
Contributor

@midN midN commented Jul 20, 2016

I don't know if you want to merge it or is it any useful for anybody else.

For me personally i found the need to get notified whenever files were synced so i know i can safely refresh page during development now.
So that's why terminal-notifier was added.

It can be made optional if there are some objections though

@EugenMayer
Copy link
Owner

could you explain, what happens, when the notifier is executed? I am absolutely interested in this feature, but could we have it, for now, as an opt int, so creating a sync-option for "notify" and then only do this, when this option is enabled?

@EugenMayer
Copy link
Owner

Ups, forgot something: Thank you for the contribution!

@midN
Copy link
Contributor Author

midN commented Jul 20, 2016

@EugenMayer - When the notifier is executed, it sends a simple Mac/Apple notification, which looks like this ( Right Top Corner):
Notifier

Agreed, sync-option sounds correct way to go, i will add it and update PR

@EugenMayer
Copy link
Owner

eventhough i like the idea, it could actually get a bit noisy, so lets make the option and go with it, great idea having those notifciations

@midN
Copy link
Contributor Author

midN commented Jul 21, 2016

@EugenMayer - Added it as an config Option, was going to make it as command-line variable, but since they are not Globally defined anywhere it seemed too extreme, so went for option thingy. I added it to example/docker-sync.yml as well

@@ -1 +1 @@
0.0.14
0.0.15
Copy link
Owner

Choose a reason for hiding this comment

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

remove this please

@EugenMayer
Copy link
Owner

that looks great already, please just adjust the changes / names and we are good too merge

@midN
Copy link
Contributor Author

midN commented Jul 21, 2016

@EugenMayer - Adjusted names and removed VERSION change

@@ -52,3 +52,5 @@ build-iPhoneSimulator/
# unless supporting rvm < 1.11.0 or doing something fancy, ignore this:
.rvmrc

# ctags
tags
Copy link
Owner

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's great to have ctags incase somebody uses them for development, in my case i use it in combination with vim + autocomplete to jump between methods efficiently and autocomplete

Copy link
Owner

Choose a reason for hiding this comment

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

i see, why not

@EugenMayer
Copy link
Owner

great contribution @midN thank you a lot!

@EugenMayer EugenMayer merged commit e0c4832 into EugenMayer:master Jul 21, 2016
@EugenMayer EugenMayer added this to the 0.0.15 milestone Jul 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants