-
Notifications
You must be signed in to change notification settings - Fork 32
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
Dropdown Select by Language #9
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR! Added a few comments. It looks like there are missing commits.
Please attach a screenshot of the resulting UI.
voices.sort((a, b) => a.lang.localeCompare(b.lang)); | ||
|
||
// Limit number of voices shown to 250 | ||
voices = voices.slice(0, 250); |
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?
@@ -28,21 +34,38 @@ const VoiceDropdown = () => { | |||
setSelectedVoice(event.target.value); | |||
setVoice(voices.find(voice => voice.name === event.target.value)); | |||
}; | |||
|
|||
const handleLanguageChange = (event) => { | |||
setSelectedLanguage(event.target.value); |
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 method doesn't exist in this PR.
))} | ||
</select> | ||
<select value={ selectedVoice } onChange={handleVoiceChange}> | ||
{voices.map((voice) => ( |
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.
Shouldn't this list be filtered by the chosen language? We should filter a new list on change and display that.
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.
Exactly
{voice.name} ({voice.lang}) | ||
</option> | ||
))} | ||
<select value={ selectedLanguage } onChange={handleLanguageChange}> |
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.
Please reformat the code using Prettier
.
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.
Sure I will do
Sorry I did a mistake, by making the pull while I am still setting up the environnement for Node (I am not familiar with how it works), while I am also trying to have ChatGPT "help"... The idea is to make a separate dropdown list for languages so it becomes easier to select... I am still trying to figure out a lot of things including how to test locally and debug, I thought that I am doing the PR on my fork... Thank you for taking time to comment, I will try to be more careful (I did but still failed) |
No worries, it's all good. :) |
No description provided.