Skip to content
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

尝试增加单词发音功能 #50

Merged
merged 6 commits into from
Feb 8, 2021

Conversation

whexy
Copy link
Contributor

@whexy whexy commented Feb 7, 2021

尝试给项目增加一个单词发音的功能

Hi, developers:

非常喜欢这个项目!我花了一些时间写了单词发音功能的雏形。调用了有道词典的发音API,看起来效果不错。

Hope you like it.

@vercel
Copy link

vercel bot commented Feb 7, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/kaiyiwing/qwerty-learner/hinw5p8mq
✅ Preview: https://qwerty-learner-git-fork-whexy-merge-pronunciation.kaiyiwing.vercel.app

@RealKai42
Copy link
Owner

非常感谢您的贡献
我简单测试了一下发现了一些bug,反复输入错误时,在上一播放还未结束时就会再次播放,造成叠加。

@sdu-gyf
Copy link
Contributor

sdu-gyf commented Feb 7, 2021

非常感谢您的贡献!
这本应是我的工作,因为个人原因一直拖着非常抱歉,请您谅解
目前发现的 bug 除了 @Kaiyiwing 提到的,还有切换词典时,会再次播放上个词典的单词

@chengluyu chengluyu added Area: Experience New functions for better experiences. Area: User Interface Animation, elements, inputs, layout, etc. Priority: High Considered to be high priority. labels Feb 7, 2021
const pronunciationApi = 'http://dict.youdao.com/dictvoice?audio='

export default function usepronunciationSound(word: string): [PronounceFunction] {
// SAFETY: We are trying to build an hook here.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个地方 eslint 报错的原因是,自定义hook没有使用规范的命名,eslint没有识别成hook
应该命名为 usePronunciationSound

@sdu-gyf
Copy link
Contributor

sdu-gyf commented Feb 7, 2021

非常感谢您的贡献!
这本应是我的工作,因为个人原因一直拖着非常抱歉,请您谅解
目前发现的 bug 除了 @Kaiyiwing 提到的,还有切换词典时,会再次播放上个词典的单词

非常抱歉这个 bug 应该是我们本身切换单词逻辑问题

@whexy
Copy link
Contributor Author

whexy commented Feb 7, 2021

Sorry for the bad code ...

反复输入错误时,在上一播放还未结束时就会再次播放,造成叠加。

提交 aefd303 修复了这个Bug。使用State包装了Audio类。

切换词典时,会再次播放上个词典的单词。

这是重复渲染造成的问题,暂时的解决方案是:切换词典后设置isStartfalse。这也许需要继续讨论。

@RealKai42
Copy link
Owner

RealKai42 commented Feb 8, 2021

Sorry for the bad code ...

反复输入错误时,在上一播放还未结束时就会再次播放,造成叠加。

提交 aefd303 修复了这个Bug。使用State包装了Audio类。

切换词典时,会再次播放上个词典的单词。

这是重复渲染造成的问题,暂时的解决方案是:切换词典后设置isStartfalse。这也许需要继续讨论。

第一种我觉得没问题,但控制台会报 warning,目前看来不影响操作。
对这个问题我一个可能但未尝试的想法是使用 useEffect 的清理旧effect的特性,在usePronunciationSound定义一个依赖于word 的useEffect,当word发生改变时,react会清理旧的effect,在清理函数中加入暂停旧播放,或许可行。 如果对 useEffect 的细节不了解,可以参考这篇文章:https://www.yuque.com/kaiyiwing/book/av0gvg。

第二个解决方案确实欠妥,如有时间的话,可以尝试其他方案。

以及ui上,我个人觉得,目前一个 喇叭状icon 和 麦克风icon 会让用户产生混乱。 其次没有引入 英音和美音的切换,我个人有个想法是,可以在hover发音功能切换时弹出下拉框,可以选择英音或者美音。 所以在ui上应该有改进空间。ui我们使用的是 css 库 tailwind,可以了解一下。

我们非常喜欢你的贡献,如果有兴趣可以一起来维护项目~

Copy link
Owner

@RealKai42 RealKai42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your excellent contribution!
目前还有改进空间,也需要对 英音美音 在ui上进行显示与切换,细节可以参考 #50 (comment)

@@ -62,12 +62,12 @@ const Word: React.FC<WordProps> = ({ word = 'defaultWord', onFinish, isStart, wo
}, [hasWrong, playBeepSound])

useLayoutEffect(() => {
if (inputWord.length === 0) {
if (isStart && inputWord.length === 0) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里不应该使用 useLayoutEffect,useLayoutEffect是与dom更新同步,而useEffect是在dom更新后进行。盲目的使用useLayoutEffect可能会引发其他问题。可以参考链接:https://zh-hans.reactjs.org/docs/hooks-reference.html#uselayouteffect

以及,对 useEffect类函数的依赖可能理解不对,这里 hooks 内部没有使用 hasWrong,可以在依赖项中移除。

@@ -162,6 +162,10 @@ const App: React.FC = () => {
(dictName: string) => {
setOrder(0)
setIsLoading(true)
// Need to stop the game, in order to prevent pronounce wrong word.
// Besides, it makes sense because users are about to stop when they change dict/chapter.
// Otherwise, introduce a new parameter to allow pronunciation begin.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这种方案可能确实很是合理的。
Need more comments @chengluyu

@RealKai42 RealKai42 mentioned this pull request Feb 8, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Experience New functions for better experiences. Area: User Interface Animation, elements, inputs, layout, etc. Priority: High Considered to be high priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants