cschiller / zhongwen

Official source code of the "Zhongwen" Chrome extension
https://chrome.google.com/webstore/detail/zhongwen-chinese-english/kkmlkkjojmombglmlpbpapmhcaljjkde
GNU General Public License v2.0
314 stars 52 forks source link

Added Zhuyin column in wordlist #30

Closed kenxjy closed 4 years ago

kenxjy commented 4 years ago

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.

image

In the example photo above, the bottom entry is an example of an older wordlist entry that is missing the Zhuyin property.

Hirse commented 4 years ago

I'm not the owner of the repo, but I like this and I have two suggestions:

  1. Instead of the dash it might be nice to show a ? icon with a tooltip that explains why this is empty.
  2. Could you (maybe in a different PR) add functionality for only showing the traditional/simplified column depending on the setting?
kenxjy commented 4 years ago

I'm not the owner of the repo, but I like this and I have two suggestions:

1. Instead of the dash it might be nice to show a `?` icon with a tooltip that explains why this is empty.

2. Could you (maybe in a different PR) add functionality for only showing the traditional/simplified column depending on the setting?

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.

kenxjy commented 4 years ago

image

Example photo of the tooltip.

Hirse commented 4 years ago

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."

cschiller commented 4 years ago

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).

kenxjy commented 4 years ago

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.

cschiller commented 4 years ago

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.

hugolpz commented 4 years ago

I added this feature in version 5.8.0. I made it so that the zhuyin attribute is never saved to local storage.

Should the pull request be closed then ?

kenxjy commented 4 years ago

I added this feature in version 5.8.0. I made it so that the zhuyin attribute is never saved to local storage.

Should the pull request be closed then ?

Good point. There's no reason to leave it open.