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

ユーザーメモ欄、提出物ページのプラクティスメモ欄でユーザーアイコンが表示されるようにした #8283

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mousu-a
Copy link
Contributor

@mousu-a mousu-a commented Jan 21, 2025

Issue

概要

変更確認方法

  1. bug/fix-user-icon-in-user-notes-fieldをローカルに取り込む。
    1. git fetch origin bug/fix-user-icon-in-user-notes-field
    2. git checkout bug/fix-user-icon-in-user-notes-field
  2. foreman start -f Procfile.devでローカルサーバーを立ち上げる。
  3. ユーザー名komagata、パスワードtesttestでログインする。(メンターであれば誰でもいいです)
  4. ユーザープロフィールにアクセスする。
  5. メンター向けユーザーメモに:@komagata:を貼り付けて保存する。(←の文字列を貼り付けるとなぜかアイコンが増殖するかもしれませんがこちらのissueと同じ問題と思われます)
  6. メンター向けユーザーメモのテキストエリアにユーザーアイコンが表示されることを確認する。
  7. 画面を再読み込みする。
  8. 同じようにユーザーアイコンが表示されることを確認する。
  9. 編集時のプレビューでもユーザーアイコンが表示されることを確認する。
  10. ユーザーの提出物詳細にアクセスする。
  11. ページ下部に4つ並んでいるタブから"プラクティスメモ"を選び、ユーザープロフィールと同様にユーザーアイコンが表示されることを確認する。

Screenshot

変更前

ユーザープロフィール
image
ユーザーの提出物詳細
image

変更後

ユーザープロフィール
image
ユーザーの提出物詳細
image

@mousu-a mousu-a self-assigned this Jan 21, 2025
@mousu-a mousu-a force-pushed the bug/fix-user-icon-in-user-notes-field branch from 7b96cb9 to 9f89c28 Compare January 21, 2025 11:39
@mousu-a
Copy link
Contributor Author

mousu-a commented Jan 24, 2025

@Ryooo-k
お疲れ様です!
よろしければこちらのレビューをお願いできますでしょうか🙏
全く急ぎではないので、ご検討いただけますと幸いです。

都合が合わなければ遠慮なく仰ってください🙇‍♂️

@mousu-a mousu-a marked this pull request as ready for review January 24, 2025 04:11
@Ryooo-k
Copy link
Contributor

Ryooo-k commented Jan 24, 2025

@mousu-a
お疲れ様です!レビューさせていただきます🫡
1週間程度お時間ください。

@mousu-a
Copy link
Contributor Author

mousu-a commented Jan 24, 2025

@Ryooo-k
ありがとうございます🙏一週間了解いたしました!
レビューでコードに関する質問などあればお気軽にお願いいたします😊

@mousu-a mousu-a requested a review from Ryooo-k January 24, 2025 11:55
Copy link
Contributor

@Ryooo-k Ryooo-k left a comment

Choose a reason for hiding this comment

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

@mousu-a
確認いたしました!コードは問題なさそうです👍
一点だけですが、キャンセル→編集→プレビューの場合、アイコンが表示されてないようなので、こちらも対応されていた方が良いかと思いました!

2025-01-29.17.29.13.mov

ご確認お願いしますー🙏

@mousu-a mousu-a force-pushed the bug/fix-user-icon-in-user-notes-field branch from 9f89c28 to 9673904 Compare January 30, 2025 13:08
@mousu-a
Copy link
Contributor Author

mousu-a commented Jan 30, 2025

@Ryooo-k

一点だけですが、キャンセル→編集→プレビューの場合、アイコンが表示されてないようなので、こちらも対応されていた方が良いかと思いました!

すみません、盲点でした👀
動画とてもわかりやすく助かりました。ありがとうございます!

修正しましたので再度レビューをお願いいたします🙏

@Ryooo-k
Copy link
Contributor

Ryooo-k commented Feb 4, 2025

@mousu-a
修正ありがとうございます!またバグ見つけましたのでご報告です💦
一度に見つけられずすみません💦

バグ1

提出物のプラクティスメモでアイコンを保存後、画面更新をするとなぜか表示されません。

2025-02-04.9.13.23.mov

バグ2

画面更新を何度も押すと、アイコンが表示・非表示で切り替わる。(切り替わるタイミングはランダム?)

2025-02-04.9.25.45.mov

プロフィールのメモはこのようなバグにはなっていないのですが、原因わかりますでしょうか?

@mousu-a
Copy link
Contributor Author

mousu-a commented Feb 4, 2025

@Ryooo-k

修正ありがとうございます!またバグ見つけましたのでご報告です💦
一度に見つけられずすみません💦

お気になさらないでくださいー!
発見ありがとうございます!!
すみません、調査します🫡

@mousu-a
Copy link
Contributor Author

mousu-a commented Feb 10, 2025

@Ryooo-k
すみません、返信が遅れてしまいました🙇‍♂️

バグの発生理由がわかりました!
ちょっと長いですが説明させてください🙏

なぜこのバグが発生しているのか

そもそもユーザーアイコンの機能は、markdown-it-user-icon.js(以下user-icon)が実行された後にuser-icon-renderer.js(以下user-icon-renderer)が実行されることで実装されています。

ただ、時々user-iconの前にuser-icon-rendererが実行されてしまうことがあり、その場合にユーザーアイコンが表示されずこのバグが発生します。(実行すべき順番が逆になってしまう)

解決策

必ずuser-iconが実行された後にuser-icon-rendererが実行されるようにする

問題点

ただこの方法には問題点があります。
user-iconはmarkdown-itというnpmのプラグインとして読み込むことで機能を実現していますが、markdown-itはasync(非同期処理)に対応しておらず、上記解決策を取ることが出来ないようです。
自分も解決を試みてみましたが、確実にuser-iconが実行された後にuser-icon-rendererが実行されるようにすることは出来ませんでした。

参考:
I need async rule, how to do it?

補足情報

user-iconとuser-icon-rendererは同じPR内で実装されたものになりますので、当時の開発者も同じように考えこの実装(時々順番が逆になってしまう状態)になっているのだと思われます。

妥協案

妥協案として、user-icon-rendererの実行をsetTimeOutで遅らせることでこの問題が解決できたらなと考えています。


すみません、手元では実装は終了しているんですがただいまテストが通らないため、落ち着いたら再レビューしていただければと思います!取り急ぎご報告でした🙇‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants