Text To Speech #30
Text To Speech #30
Conversation
|
@MBJean Thank you for all your comments. They have all been addressed and updated. |
|
Thanks, @taokinbo! I intend to take another pass at this today and leave a few more comments, but that'll conclude my review. |
| given_tok = nltk.tokenize.word_tokenize(given_sent.lower().translate( | ||
| str.maketrans('', '', string.punctuation))) | ||
| correct_tok = nltk.tokenize.word_tokenize(correct_sent.lower().translate( | ||
| str.maketrans('', '', string.punctuation))) |
There was a problem hiding this comment.
As a takeaway for the future, it's always worth considering how to make your code as readable as possible for the next developer. These long chained method calls with nested chained method certainly take me a second to parse fully, which is probably the impulse behind leaving a comment here. In this case, we could pull out the maketrans dictionary to improve readability:
translate_dictionary = str.maketrans('', '', string.punctuation)
given_tok = nltk.tokenize.word_tokenize(given_sent.lower().translate(translate_dictionary))
correct_tok = nltk.tokenize.word_tokenize(correct_sent.lower().translate(translate_dictionary))
Or even increase the verbosity even further to improve readability:
translation_dictionary = str.maketrans('', '', string.punctuation)
given_sanitized_sentence = given_sent.lower().translate(translation_dictionary)
correct_sanitized_sentence = correct_sent.lower().translate(translation_dictionary)
given_tokenized_sentence = nltk.tokenize.word_tokenize(given_sanitized_sentence)
correct_tokenized_sentence = nltk.tokenize.word_tokenize(correct_sanitized_sentence)
The above pattern does two things:
- Allows the next developer to skim more quickly and, in effect, ignore the right side of the expression. The expressively named variables tell me more about the method used to get to the end result of these transformations without even needing to read the implementation and they make the exact pieces of the implementation easier to find in the event that I do need to investigate the implementation.
- Removes the need for an explicit comment. This is a personal opinion, but I often find comments are ways of getting around writing expressive code.
| continue: false, | ||
| showAnswer: false, | ||
| }); | ||
| document.getElementById('content').value = ''; |
There was a problem hiding this comment.
This and the other places where we manually grab the value of the textarea with id="content" stand out to me. At least as far as I can tell so far, a more conventional approach would be to have an onChange={} listener on the <textarea> that sets the event.target.value to state.userText. That way, you always have an up-to-date match of what the user is typing rather than having to use vanilla JavaScript document.getElementById to reach into the DOM and manually grab the value when you need it.
Did you consider that more conventional approach? Did you encounter any problems with it?
In general, the approach you've used here mixes React's declarative style with a more imperative style, and in general it benefits both development and the user experience to stick to the former.
| {playButton('pink')} | ||
| </div> | ||
| <div className="play-button-buffer"> | ||
| {null} |
There was a problem hiding this comment.
What's the purpose of this null?
| </div> | ||
| <div className='row'> | ||
| <div className="col box"> | ||
| <form id='input-form'> |
There was a problem hiding this comment.
This form might need an onSubmit={(event) => event.preventDefault()} or something like that. Without it, I assume that we'll leave in default form behavior for the enter key press (i.e., it'll submit the form with no specified action and so reload the page).
| onClick={this.handleSubmit} | ||
| disabled={this.state.continue}> | ||
| Submit | ||
| </button> |
There was a problem hiding this comment.
I think this and the subsequent <button>s have the wrong form attribute. Looks like they might have a holdover from another component picturebook-form rather than pointing to the input-form.
MBJean
left a comment
MBJean
left a comment
There was a problem hiding this comment.
Thanks for this, all! I've had a chance to give a pass-through to the whole change and have left a bunch of comments above. @taokinbo, thanks for addressing the first half of those already. I've left a few more throughout. Please let me know if you have any questions.
|
Just a quick note, all! Since we're past the end of the semester, you're under no obligation to address the feedback I've left above. Please do read through it and let me know if you have any questions, but Ryaan and I will take care of wrapping this PR up and getting it into production. Thanks! |
We created a text to speech learning activity:
the answer
@ryaanahmed @MBJean