Closed omarbakeer closed 5 years ago
Hello James, It seems like my first PR didn't go well, although it adds a trivial feature :cry: My apologies I haven't done the requested changes, I didn't get any notifications by github, so I was just passing by my repos and found out all this mess :man_facepalming:
Hello James, It seems like my first PR didn't go well, although it adds a trivial feature π’ My apologies I haven't done the requested changes, I didn't get any notifications by github, so I was just passing by my repos and found out all this mess π€¦ββοΈ
It's okay, don't worry - PRs rarely get approved first time. I'm sure we can work this into a nice addition to the module. π I tested it manually (I've not got round to mocking the Web Speech API in automated tests - this might be a fun challenge) and it worked as expected with and without options.
I'm trying to imagine the use case for discontinuous listening. In my mind, it would be for an interface where the user needed to press a button to issue a voice command. Every time they pressed the button, the previous command would be erased.
In that particular use case, there are two improvements you could make:
resetTranscript()
before startListening()
. However, we could save them the trouble by resetting it automatically when starting a new discontinuous listen. i.e. before starting up the speech recognition on line 112, you could do something like:
if (!recognition.continuous) {
this.resetTranscript();
}
listening
boolean prop. When they stop speaking on a discontinuous listen, this should change to false. Currently, it doesn't do that - we just need to make sure listening
gets updated on discontinuous disconnects. So onRecognitionDisconnect
could be changed to:
onRecognitionDisconnect() {
listening = false;
if (pauseAfterDisconnect) {
this.setState({ listening });
} else if (recognition) {
if (recognition.continuous) {
this.startListening();
} else {
this.setState({ listening });
}
}
pauseAfterDisconnect = false;
}
If you did something like this, you should update the startListening
documentation to state that it behaves differently when the continuous
option is set to false.
If the voice command button was the use case you had in mind, it'd be good to suggest it in your docs for the continuous
option. Alternatively, you could create a new section in the README
for "Suggested use cases", explaining how to use react-speech-recognition
for each one. The default settings perhaps combined with the autoStart
option set to false and buttons for starting/stopping dictation could be used for a note-taking use case. Your continuous
option plus the listening
prop could be used for a voice command/search kind of feature. Just an idea.
Could you clarify the use case you had in mind?
Yes exactly James, this is the use case I wanted to cover.
Recently, I was working on a chatbot that could take user input either by text or speech. And I found this very handy HOC you made to encapsulate the Web Speech API's. So for the chatbot use case, the user doesn't expect the bot is continuously listening to him, obviously because the chat is a two way communication, instead I had to provide a button/trigger to initiate the listening.
I didn't think about other use cases actually, but that was it for me, and I think it's pretty useful to expose ( if there is any other ) Web Speech API features to developers, so they stop being concerned about the module implementations, or if it's missing anything from Web Speech API.
Is it missing something yet ? I guess now it needs you to manually merge the pr, I believe my last commits covered your requested modifications, right ?
Is it missing something yet ? I guess now it needs you to manually merge the pr, I believe my last commits covered your requested modifications, right ?
Ah sorry, Omar - I didn't notice you had committed new changes. I've made a note to review more thoroughly within the next day or two.
@omarbakeer I've made two minor comments. Once they are addressed, I'll merge and publish this.
@omarbakeer I've made two minor comments. Once they are addressed, I'll merge and publish this.
I've covered those comments. I hope you understand that this is my first contribution ever :astonished: , and sorry if I messed it up or forgot about anything, actually I would like to thank you giving me this opportunity to collaborate :smiley: :fireworks: :confetti_ball:
@omarbakeer I've made two minor comments. Once they are addressed, I'll merge and publish this.
I've covered those comments. I hope you understand that this is my first contribution ever π² , and sorry if I messed it up or forgot about anything, actually I would like to thank you giving me this opportunity to collaborate π π π
Congratulations Omar, your first GitHub contribution is live on npm as version 1.1.0! It's a useful feature - thanks for adding it. π
I've added the new option to enable users to control the API. Using a new option property, users can enable/disable the continuous listening of the API. I kept the default configs to be continuous and made the necessary modifications that will add the feature.