-
Notifications
You must be signed in to change notification settings - Fork 131
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
Transcriptions in the new sentence design #2224
Conversation
Temporary solution for #1087.
59fbd19
to
fcaf212
Compare
I’m very happy about that! 🎉 I havn't reviewed the code yet so I'll just comment on the visible part first.
It looks like your implementation is a little bit different. Consider a Japanese sentence with reviewed furigana. I have the option "Always show transcriptions" turned on. I go on the sentence page. In the old design, the furigana appears on the top of the sentence. In the new design, it appears below. Another problem: when the furigana is on the top, I can’t see figure out how to edit the transcription.
I think it’s okay. I realized in UX test 3 that some of these icons needed to be changed anyway.
That would probably annoy learners of Japanese who don’t have account. The transcriptions button on the right are easily seen as belonging to a single group. It is not very intuitive which button is acting on what. Probably gray background on hover makes it even more non intuitive. I guess it’s a similar feeling to what agrodet mentioned about eye-matching the (i) icon with the sentence text. I have no idea right now on how to improve that but I’ll think about it. I think I would look more consistent if the transcription text (if reviewed) is aligned with the sentence text. |
When showing the hidden transcriptions, also expand the translations in case some of the hidden translations have hidden transcriptions.
Showing edit button also when user can edit the transcription and hiding language and sentence fields if user can only edit the transcription.
This should be fixed.
The best solution I have for this is to combine the editing of the transcription with the editing of the sentence. I had already considered doing it since the beginning but it complexifies the "Edit" feature so I was a bit reluctant to go for it. But I also really don't like having these extra buttons next to the transcription text. One consequence is that it's no longer possible to edit the transcriptions of translations. I think users will be okay with that. They cannot edit the translations text so it shouldn't feel too annoying that they can no longer edit the transcriptions of translations. They probably rarely did it anyway. One other consequence is that it takes more steps to mark a transcription as reviewed. It requires to first click on the "Edit" button then click on the check icon next to the transcription in the sentence form. But I'd rather go for a longer term solution here rather than try to squeeze one more button in the sentence component view. The ideal solution, I think, would be to have page "Review transcriptions" where people can efficiently proofread transcriptions. My implementation does not take into account yet that there can be several editable transcriptions for a same sentence. It will probably have to be implemented as part of #2189.
I have added a button for non-authenticated users. For lack of a better idea, I reused the same button as for expanding or collapsing the sentence menu.
This should be fixed. |
Good job! I think you solved the buttons problem in a smart way. 😸 I’m still commenting the visible part only. Problem: After I click the review button of a transcription, the tooltip of that transcription says that it has been generated by a piece of software. Only after I reload the page, the tooltip correctly says "last edited...". Problem: On a Japanese sentence, I click on the edit button, I modify both the sentence text and the transcription in a way that makes the transcription invalid. I click save, the sentence is saved, but the transcription isn’t saved and the edition box stays open, showing the autogenerated transcription. Instead, it should show me an error and prefill the transcription field with what I last entered. By the way, do we want to allow editing both sentence and transcription? I can imagine a scenario when the autogenerated transcription is known to be wrong and the user wants to both edit the sentence and the transcription to avoid fixing every single place the autogenerated transcription would be wrong after editing the sentence. Your current implementation allows that, which I think is great.
I think it’s good enough. 👍 On a side note, I think this change may make the "always show transcriptions" setting less useful, because people could now quickly show all transcriptions on a page by clicking on "expand all menus for all sentences". Since this "expanded state" is remembered across pages, it could almost replace the "always show transcriptions" setting. I am not sure we want users to use the "expand" feature in this way though. |
The position of the tooltips feels a bit off on this sentence. The tooltip over the simplified characters is right on the middle of the text, however on the pinyin it’s off on the right. Since the pinyin spans on two lines, the tooltip is positioned on the center of the card, even though there is quite some blank on the right. |
We have to add the Users as containment to retrieve the user to display in the tooltip.
It's supposed to display an error message, not replace the transcription by the auto-generated one.
I fixed the two problems you mentioned.
Yes, we definitely want to allow it. Editing the transcription is a way to mark it as reviewed.
This way of marking the transcription as reviewed is not necessarily ideal. If I make significant changes on the sentence, I may want to first get the auto-generated transcription and then apply the necessary fixes on top of it. But that can be an improvement for another time.
It's still a bit too early to tell but I think I wouldn't be against using the "expand" feature in this way.
I changed the position of the tooltip to be above instead of below the transcription. |
Could you make PinyinHelper and TranscriptionsHelper use your new PinyinTrait and FuriganaTrait so that we avoid keeping duplicate code? |
Thanks for the quick update, Trang. Please have a look at my new comments. 👆 I think your implementation is feature-complete, except that the detailed error messages are not displayed when an invalid transcription is submitted. I think these detailed error messages are quite necessary because a number of autogenerated transcriptions are invalid already (in particular Japanese sentences that include numbers). It means an error can be produced just by clicking on the "Mark as reviewed" button. |
Using the traits to avoid duplicate code.
The comments have been addressed 🙂 |
Thank you, Trang! 👍 |
The issue happens if there are errors in the transcription and the users cancels. Cancel relies on `oldSentence` but by using initSentence() we override the `oldSentence`.
Thanks for the update, Trang. I think the error handling part may be improved a bit but I’ll merge it now so that UI translators can start working already. We can continue to work on it in the dev branch. |
This PR implements the transcriptions in the new sentence design.
For the major part, it works the same as in the old design.
Things that changed
Todo for another time