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 -contentImage option to TerminalNotifier #98

Merged
merged 2 commits into from Apr 24, 2024
Merged

Conversation

vesper8
Copy link
Contributor

@vesper8 vesper8 commented Mar 7, 2024

Related to #73

Since appIcon is no longer supported, there is this alternative to including an image using -contentImage and this PR adds the ability to set the appImage as an option.

Copy link
Member

@pyrech pyrech left a comment

Choose a reason for hiding this comment

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

Thanks for you PR 🤗

Could you add a note in the CHANGELOG and a test?

@pyrech
Copy link
Member

pyrech commented Mar 7, 2024

Do you have some link explaining why appIcon is no longer supported on TerminalNotifier? If this is a dead option, should we now pass it to the keep -contentImage option directly (without adding a new contentImage in the notification)?

@vesper8
Copy link
Contributor Author

vesper8 commented Mar 7, 2024

Only link I have is the issue I linked in the post above where you yourself suggested The only thing here we can do here is to change a bit the behaviour of terminal-notifier to embed an image instead of trying to change the notification's icon by using -contentImage instead of -appIcon

I tried your solution and contentImage does indeed allow us to pass an image or icon, and the appIcon appears to do nothing at all. So yes I guess it does look like it's a dead option now.

Related to jolicode#73

Since appIcon is no longer supported, there is this alternative to including an image using -contentImage and this PR adds the ability to set the appImage as an option.
@pyrech pyrech force-pushed the patch-1 branch 3 times, most recently from d1c75b8 to 89a9938 Compare April 24, 2024 20:13
@pyrech
Copy link
Member

pyrech commented Apr 24, 2024

Hi @vesper8 I changed a bit your PR to make use of terminal-notifier's contentImage option instead of appIcon, so no need to support a new option for users. Thanks for your initial work on this PR 🙂

@pyrech pyrech merged commit 3963656 into jolicode:main Apr 24, 2024
4 checks passed
@vesper8
Copy link
Contributor Author

vesper8 commented Apr 29, 2024

@pyrech Thank you : )

New tagged release on the way? <3

@pyrech
Copy link
Member

pyrech commented Apr 29, 2024

I will tag a new version in a few days, I would like to merge some PRs before ;)

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

2 participants