Added Zhuyin column in wordlist #30
Conversation
added zhuyin support in wordlist
js/wordlist.js
Outdated
|
|
||
| let wordList = localStorage['wordlist']; | ||
| let zhuyinConfig = (localStorage['zhuyin'] === 'yes'); |
There was a problem hiding this comment.
Maybe rename the variable to showZhuyin or zhuyinVisible.
|
I'm not the owner of the repo, but I like this and I have two suggestions:
|
I changed the default text to a tooltip that explains what happened. That was a good idea. As for a setting that only shows traditional/simplified column, that's also not a bad idea. Perhaps in the future. |
|
Example photo of the tooltip. |
|
Thanks for the changes. As far as I know, the wordlist page is already using Bootstrap. As such, I would recommend using their tooltip implementation so you don't have to add one yourself (and to keep it consistent). For the tooltip text, I would be more specific like: "Zhuyin was added to the wordlist after this entry was saved and is only available for new entries." |
|
I think if the Zhuyin column is present it should always show a value in every row. There should not be any missing Zhuyin values. This behavior would be really confusing to the user. Since Zhuyin is derived from Pinyin it should be fairly easy to determine it on the fly. In this case there would also be no need for a tooltip. As for only showing traditional or simplified characters, I believe this only adds unnecessary complexity. The main purpose of the built-in word list is that the dictionary entries can be exported into a tool like Anki. For example, if you don't like simplified characters, then you just don't map this column in the Anki importer. I don't think people will use the word list itself for studying. It's just meant to be used as an exporter for the CC-CEDICT dictionary entries you're interested in studying (plus the derived Zhuyin for convenience, maybe). |
|
It should generate Zhuyin from the Pinyin. This will avoid having to use the tooltip for missing Zhuyin values, and like previously mentioned, avoid any confusion for the user. I decided any generated Zhuyin get saved to the wordlist, this way it doesn't need to be generated again. |
zhuyin.js
Outdated
| return zhuyin; | ||
| } | ||
|
|
||
| let zhuyinTones = ['?', '', '\u02CA', '\u02C7', '\u02CB', '\u30FB']; |
There was a problem hiding this comment.
Can this be const?
(Actually applies to all let in this file I think.)
manifest.json
Outdated
| @@ -49,6 +50,7 @@ | |||
| "web_accessible_resources": [ | |||
| "css/*", | |||
| "js/*", | |||
| "images/*" | |||
| "images/*", | |||
| "zhuyin.js" | |||
There was a problem hiding this comment.
Maybe it makes more sense to move it to js/zhuyin.js.
js/wordlist.js
Outdated
| let tone = getToneNumber(py); | ||
| let syllable = removeToneMarker(py, tone) + tone; |
There was a problem hiding this comment.
I feel like these to calls could be combined in one function (pinyinToSyllable for example).
Right now it was a bit confusing to read.
js/wordlist.js
Outdated
| syllables.push(syllable); | ||
| }); | ||
|
|
||
| let zhuyin = syllables.reduce((zhuyin, syllable) => zhuyin + mapToZhuyin(syllable) + ' ', '').trimEnd(); |
There was a problem hiding this comment.
This line is also in content.js.
Can you extract it to the zhuyin file?
js/wordlist.js
Outdated
| @@ -3,6 +3,13 @@ | |||
| Copyright (C) 2010-2019 Christian Schiller | |||
| https://chrome.google.com/extensions/detail/kkmlkkjojmombglmlpbpapmhcaljjkde | |||
| */ | |||
| const utones = { | |||
There was a problem hiding this comment.
Why utones?
Are these the markers for the tones in pinyin?
content.js
Outdated
| let utones = { | ||
| 1: '\u0304', | ||
| 2: '\u0301', | ||
| 3: '\u030C', | ||
| 4: '\u0300', | ||
| 5: '' | ||
| }; | ||
| // let utones = { | ||
| // 1: '\u0304', | ||
| // 2: '\u0301', | ||
| // 3: '\u030C', | ||
| // 4: '\u0300', | ||
| // 5: '' | ||
| // }; |
There was a problem hiding this comment.
Thanks for your changes. 👍
I'm not sure about moving these though.
They seem to be used in non-zhuyin contexts too so they don't necessarily fit into that file.
Maybe @cschiller has a decision whether to include them.
(Either way, commented code should be removed.)
There was a problem hiding this comment.
Oops. I left the comments in! Doh!
There was a problem hiding this comment.
Thanks for your changes. +1
I'm not sure about moving these though.
They seem to be used in non-zhuyin contexts too so they don't necessarily fit into that file.
Maybe @cschiller has a decision whether to include them.
(Either way, commented code should be removed.)
I was wondering about this too. I was trying to avoid duplicate variables that contain the same constants.
Edit: typo.
|
I added this feature in version 5.8.0. I made it so that the zhuyin attribute is never saved to local storage. It is always generated on the fly. This saves some space and it allows for bugfixes if a mapping problem is discovered. |
Should the pull request be closed then ? |
Good point. There's no reason to leave it open. |
I quite like using this extension, however, I personally use both Zhuyin and Pinyin for learning Mandarin. I actually prefer Zhuyin as it helps me distance myself from English. I find it helps me with my pronunciation.
I like the wordlist feature, but I wished it had a Zhuyin column to help me copy over stuff to Anki more easily. So... I implemented it myself, and I figured I would share this for anyone else wanting this feature as well.
I reused the "Show Zhuyin phonetic symbols" option to toggle displaying the Zhuyin column in the wordlist. It is not backwards compatible with any previously saved wordlist, it will just show a dash as it's Zhuyin.
In the example photo above, the bottom entry is an example of an older wordlist entry that is missing the Zhuyin property.