-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[16.0][IMP] web_notify:t3894 Allow notify with sound #2923
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
[16.0][IMP] web_notify:t3894 Allow notify with sound #2923
Conversation
e1ca00d to
d8630d6
Compare
ivs-cetmix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional review LGTM
| * specified settings like volume and looping. It also provides the ability | ||
| * to trigger actions when the audio playback ends. | ||
| */ | ||
| export class AudioPlayer extends Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using a service like SoundEffects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this service is limited to predefined sound effect settings and does not allow you to specify a link to any sound. In my case, only the sound effect link is passed for playback, which allows any sound to be dynamically played back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I mean, you can take inspiration in that code to implement your own sound service, resulting in a simpler impletation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chienandalu I was already inspired by this effect to write the current solution :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I mean, you can take inspiration in that code to implement your own sound service, resulting in a simpler impletation
Hi @chienandalu, hope you are doing well! Would appreciate your review in order to proceed further.
d8630d6 to
77e3a53
Compare
0a1ad0d to
50e7956
Compare
|
Hi @chienandalu could you please review the code again? Thank you in advance! |
|
Hi @rvalyi , would appreciate you finding some time to review this. Really would be great to have it merged. |
50e7956 to
802b34a
Compare
|
Hi @chienandalu @rvalyi would appreciate your review! We are using this module in several project already, I think community would definitely benefit from it too. |
chienandalu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just code review 👍
|
This PR has the |
thank you @chienandalu ! |
Bearnard21
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@CarlosRoca13 Could you please merge the PR? Thank you. |
CarlosRoca13
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ocabot merge minor
|
On my way to merge this fine PR! |
|
Congratulations, your PR was merged at 06ef262. Thanks a lot for contributing to OCA. ❤️ |
These changes allow you to send notifications with an audible alarm.