-
Notifications
You must be signed in to change notification settings - Fork 12
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
#6-testing-text-to-speech (REVIEW) #14
Conversation
@@ -46,6 +47,8 @@ export const TestVerbFormComponent: React.FC<Props> = props => { | |||
onNextQuestion(); | |||
}; | |||
|
|||
const { speak, voices } = useSpeechSynthesis(); |
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.
I think you have missing the culture (mine is speaking with spanish pronunciation :))
en-uk - Daniel
sounds quite good
https://mikeyparton.github.io/react-speech-kit/
Can you give a try and configure it?
Thanks
Moved everything audio related to a new component and allowed for voice selection (only for english speaking voices). Component doesn't show if not supported, according to API that will be only IE and Opera for Android. Don't know if we should add an alternative audio solution. |
There's a catch first time there's no voice selected, if you click on pronunciation it doesn't sound, could you just choose one by default? (e.g. Daniel Uk) ? |
There's a problem with that, the list of voices depends on how you are connecting to the app. According to the web api doc:
In fact, I am not getting that "Daniel UK" option you mention, and I get different options with different browsers. The only solution I can think of (coded on last commit!) is loading the voices list (won't be empty, since that case is already checked on 'supported' constant) and selecting the first one of the english options by default. But it will be different for each user. |
setEnglishVoicesArray( | ||
voices.filter(voice => { | ||
return voice.lang.substring(0, 2) === 'en'; | ||
}) |
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.
We have to work in a defensive way here, many scenarios can arose and application could become buggy or crash.
Let's do the following thing:
- What would happen if there's no 'en' voice ? It should be empty, then we should disable or hide the play button.
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.
Here instead of choosing 0, for english voices I would search for the first voice that is en-gb, in case it doesn't exists fallback to english-us if not then take the first one that is available
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.
I would extract to a business method and unit test it using jest
Last commit:
|
Cleaned up and integrated in the final app in a different branch, thanks for the guide :) |
Info on my last issue comment. #6 (comment)
Forgot to pull request to let you know about review.