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

リンクカードを実装 #8139

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

Conversation

mousu-a
Copy link
Contributor

@mousu-a mousu-a commented Oct 19, 2024

Issue

概要

新機能リンクカードを実装しました。

変更確認方法

  1. feature/introduce-of-link-cardをローカルに取り込む。
    i. git fetch origin feature/introduce-of-link-card
    ii. git checkout feature/introduce-of-link-card
  2. rails db:seedでテストデータを作成。
  3. foreman start -f Procfile.devでローカルサーバーを立ち上げる。
  4. ユーザー名komagata、パスワードtesttestでログインする。(誰でもいいです)
  5. 日報一覧にアクセスする。
  6. 最新の日報("リンクカードの動作確認")を開く。
  7. 正しいパターンでのみリンクカードが表示されていることを確認する。

Screenshot

変更前

image

変更後

image

@mousu-a mousu-a self-assigned this Oct 19, 2024
@mousu-a
Copy link
Contributor Author

mousu-a commented Oct 19, 2024

@machida
お疲れ様です。
お手すきの際にリンクカードの動作確認をお願いいたします。

Tweetのデザインの適用のさせ方

こちらのリンク

HTML中のscriptタグを実行することで、blockquoteタグがiframeタグに置き代わり、埋め込みツイートのデザインが適用されています。

にあるように、レスポンス内のscriptタグを実行することでTweetのデザインを適用させていますが、
このようなissueがあり、後々この方法は使えなくなるかもしれません。

@machida machida force-pushed the feature/introduce-of-link-card branch from 0eaf7d4 to 783adac Compare October 21, 2024 17:54
@machida
Copy link
Member

machida commented Oct 21, 2024

@mousu-a

このPRに最新のmainを取り込みました。
手元で

git pull --rebase origin feature/introduce-of-link-card

をお願いします。


このブランチにプルリクを送りました。
#8143

デザイン以外にJSに手を入れてるので、それらを確認の上(コードを読んだり、動作を確認したり)、OKだったらこのブランチにマージしてくださいー

もし、修正が必要なときは、

もしくは、

どちらかでお願いします。

@mousu-a
Copy link
Contributor Author

mousu-a commented Oct 22, 2024

@machida
ありがとうございます!!😭

@machida machida force-pushed the feature/introduce-of-link-card branch from b3f1aba to c3954c8 Compare November 6, 2024 15:51
@machida
Copy link
Member

machida commented Nov 6, 2024

?なんかコミットの履歴がおかしい

修正しました!
手元で動かすときは、

一旦手元のブランチ feature/introduce-of-link-card を削除して、再度 origin から持ってきてください。

git branch -D feature/introduce-of-link-card
git fetch origin feature/introduce-of-link-card
git checkout feature/introduce-of-link-card

@machida machida force-pushed the feature/introduce-of-link-card branch 2 times, most recently from 4ff3c5f to 7ca62e1 Compare November 6, 2024 16:02
@machida
Copy link
Member

machida commented Nov 6, 2024

@mousu-a

## A

あいうえお@[card](https://bootcamp.fjord.jp)かきくけこ

---

## B

あいうえお @[card](https://bootcamp.fjord.jp)かきくけこ

---

## C

あいうえお@[card](https://bootcamp.fjord.jp) かきくけこ

---

## D

あいうえお
@[card](https://bootcamp.fjord.jp)かきくけこ

---

## E

あいうえお@[card](https://bootcamp.fjord.jp)
かきくけこ

---

## F

あいうえお
@[card](https://bootcamp.fjord.jp)
かきくけこ

---

## G

あいうえお

@[card](https://bootcamp.fjord.jp)
かきくけこ

---

## H

あいうえお
@[card](https://bootcamp.fjord.jp)

かきくけこ

---

## I

あいうえお

@[card](https://bootcamp.fjord.jp)

かきくけこ

これを貼り付けると、GとIだけがリンクカードになります。
本当はIのときだけリンクカードになってほしかったのですが、これでいいかなーという感じです。

@mousu-a
Copy link
Contributor Author

mousu-a commented Nov 7, 2024

@machida
ありがとうございます🙇‍♂️
確認させていただきます!

@mousu-a
Copy link
Contributor Author

mousu-a commented Nov 8, 2024

@machida
例までありがとうございます!とても助かります🙇‍♂️
Iのときだけリンクカードに出来ないかちょっと調べてみます🙏

@mousu-a mousu-a force-pushed the feature/introduce-of-link-card branch from 7ca62e1 to 7a9b10f Compare November 12, 2024 01:19
@mousu-a
Copy link
Contributor Author

mousu-a commented Nov 12, 2024

@machida
お疲れ様です。

Iの時だけリンクカードを生成するようにコードを変更しました。合わせてフィクスチャも変更しています。
お手すきの際にご確認ください🙇‍♂️
(せっかくいただいた修正を上書きする形となってしまい、申し訳ないです🙇‍♂️)

ZENNのリンクカード実装がほとんどそのまま使えたのでそのまま使っています。(FBCの仕様に合わせて改変を加えています)


今回実装するリンクカードの仕様として「マークダウンにネストした状態でもリンクカードを生成する」となっていますが、これはdetailsだけ許容する形としています。
例えばmessage記法の中ではリンクカードを展開しません。認識に違いがあればご指摘いただけますと幸いです🙇‍♂️


ところでリンクカードの前後に中身のない空のpタグが表示されてしまい、いまだ原因が掴めません...
こちら出来ればアドバイスなどいただきたいです🙇‍♂️

リンクカードの前後に謎のpタグが入ってしまう

現状説明

このようにリンクカードを展開すると<div class = "a- link-card">の前後に中身のないpタグが挿入されてしまっています。

Pasted_Graphic

私見、試してみたことなど

tokensの配列としては"paragraph_open", "html_inline", "paragraph_close"となっており、余計なpタグが入る余地はないと思うのですが...

Pasted_Graphic_1

確認してみたところ、リンクカードの対象となるToken(tokens[i])の前のpタグはどうやらtokens[i - 1]のようです。
tokens[i - 1].attrJoin('style', 'display: none')とすると、リンクカードの前のpタグにdisplay: noneが適用されます。

Pasted Graphic 2 ただ後ろのpタグはtokens配列の中に存在しません。(存在しないのにpタグが生成されてしまっているようです)

こちらアドバイスなどいただけたらと思います🙇‍♂️

@machida
Copy link
Member

machida commented Nov 12, 2024

@mousu-a markdownの仕様に従ってmarkdown記法からHTMLを生成したときに発生してるのかもです。もしかしたら、pの開始タグだけ生成されてしまって、それをChromeがHTMLの矛盾を無くすために無理やりpを閉じている、とかかもしれないです。念のためFFとSafariでも確認してみるといいです。もしかしたら別のHTMLになっているかもです。

そのまま実装したらリンクカードがpで囲われてしまうのを、そのpを生成しないという機能も付けてるので、それとのバッティングなどもあるかも?

で、僕もその辺の問題がうまく解決出来なかったんですよねー。なので、Iのときだけリンクカードにするというのは、そこまで優先度は高くないので、今回の件が解決ができなかったら、前のコードでレビューを出してしまって大丈夫です。

ただ、変なHTMLに変換されないようにしたいので、コードを戻してもその点はチェックをお願いしたいです。

@mousu-a
Copy link
Contributor Author

mousu-a commented Nov 14, 2024

@machida
お疲れ様です。
空のpタグが出てしまう問題が解決出来ました!
アドバイスありがとうございました🙇‍♂️
Pasted Graphic 5

正確には、Markdown-it プラグインを適用する時に元となったa要素(の親であるp要素)をdisplay: noneにし、replace-link-to-card.jsでリンクカードを生成する際に削除する形となっています。
image

なぜリンクカードの前後に空のpタグが出力されてしまっていたのか

コードのコメントにも書いてありますが、 tokens[i](リンクカード記法のToken)の位置に割り込む形でpushしてしまうと、Tokenの配列として正しい形でなくなり、正しくないHTMLが出力されてしまっていたようです。
(tokensの最後にpushすると余計なpタグは出てきませんでした)
なので、リンクカード記法のToken(pOpen inline pClose)の後ろにpushすることで解決としています。


お手すきの際にご確認をお願いいたします🙇‍♂️


すみません、余計なdiffが混じってしまいました。ここは本質的な変更には関係ありません。
コミットを分けるべきでした🙇‍♂️
4b3286b#diff-fd96d68161d49a2363a82d7312147b1395cff86b00fe40b27fed62ec83b1d1afR15

@machida
Copy link
Member

machida commented Nov 15, 2024

@mousu-a
動作確認しました!!
バグを見つけましたので確認をお願いします。

@[card](https://esa.io/)

@[card](https://www.yahoo.co.jp/)

@[card](https://bootcamp.fjord.jp/)

このように、文章の最後がリンクカード記法になっている場合、最後のリンクカード記法がリンクカードに変換されませんでした。

@[card](https://esa.io/)

@[card](https://www.yahoo.co.jp/)

@[card](https://bootcamp.fjord.jp/)

aaaaa

上記だと変換されます。


デザインやクラス名を少し修正しました。

@mousu-a
Copy link
Contributor Author

mousu-a commented Nov 15, 2024

@machida
動作確認、デザインの調整ありがとうございます!
すみません、バグの方確認します🙇‍♂️

@machida
Copy link
Member

machida commented Nov 15, 2024

@mousu-a調査よろしくお願いします。そこ以外の動作はバッチリでした👍

@mousu-a mousu-a force-pushed the feature/introduce-of-link-card branch from 788ae1f to 7920338 Compare November 16, 2024 04:25
@mousu-a
Copy link
Contributor Author

mousu-a commented Nov 16, 2024

@machida
バグ解決出来ました!
再度動作確認の方お願いいたします🙇‍♂️


バグ

3つ以上リンクカードが生成されるとき、最後のリンクカードだけ生成されていませんでした。

原因

リンクカードを生成する際、html_blocktokens配列にpushしていたことにより、tokensの要素数がズレ、そのズレによって最後の方のtoken(要素)が(html_blockをpushしてズレた数だけ)処理出来ず、最後のリンクカードが生成できていなかったようです。

解決

for文を使うことにより、tokens配列の要素数の増減に合わせて処理を行えるようにしました。


すみません、rebaseしたのでこちらをお願いいたします🙇‍♂️
git branch -D feature/introduce-of-link-card
git fetch origin feature/introduce-of-link-card
git checkout feature/introduce-of-link-card

@machida
Copy link
Member

machida commented Nov 19, 2024

@mousu-a

確認しました!!
ほぼOKだったのですが、

@[card](https://bootcamp.fjord.jp/articles/146)
@[card](https://bootcamp.fjord.jp/articles/146)

上記のとき、「あ」が無視されリンクカードが生成されてしまいました。
小さなバグなので目をつぶってもいいかもですが、これも直せるとよりいいです。

@mousu-a
Copy link
Contributor Author

mousu-a commented Nov 19, 2024

@machida
ありがとうございます!!
すみません、正規表現見直します!

@machida
Copy link
Member

machida commented Nov 19, 2024

@mousu-a 了解です!よろしくお願いしますー🙏

@mousu-a mousu-a force-pushed the feature/introduce-of-link-card branch from 7920338 to f5a7e93 Compare November 21, 2024 00:19
@mousu-a
Copy link
Contributor Author

mousu-a commented Nov 21, 2024

@machida
お疲れ様です。
再度ご確認のほどよろしくお願いいたします🙇‍♂️

以下行った変更です。

  • リンクカード記法の末尾に文字がある場合もリンクカードを生成しないようにしました。
  • New:URLがただの文字列の場合リンクカードを生成しないようにしました。
  • New:URLのスキームが http / https以外の場合もリンクカードを生成しないようにしました。
  • フィクスチャのカバレッジをあげました。
  • マークダウンのネストについて、とりあえずはmessageとdetailsだけ許容する形としています。

@mousu-a
Copy link
Contributor Author

mousu-a commented Nov 28, 2024

@machida
お疲れ様です!
すみません、ついZENNの実装に引っ張られて、現在の実装ではマークダウンのネストはmessageとdetailsだけ許容する形としています。
MTGでもmachidaさんにご確認いただき「とりあえずはそれでリリースして良い」とのことでしたが、FBCのリンクカードの仕様としては「マークダウンにネストした状態でもリンクカードは展開する」というものでした。(すみません、いつの間にか頭から抜けていました🙇‍♂️)

聞きたいこと

今からでもそのように(全てのマークダウンにネストできるように)修正した方がいいでしょうか?(修正の手間はそれほどかかりません)


また、その際修正する考え方としては、"p要素にネストしていなければ基本的にはリンクカードは展開する"という形で良かったでしょうか?
(p要素にdiv要素(リンクカードのHTML)がネストしてしまうとHTMLとしてinvalidであるため)

@machida
Copy link
Member

machida commented Nov 29, 2024

@mousu-a
今のままで大丈夫ですー
detailsやmessageの中以外では展開してしまうとほとんどの場合invalidになっちゃうんですよね。ほとんどの場合と言ったのは、liだけ例外になるからです。

liの中で展開してもinvalidにはなりません。なので、もしliの中でだけリンクカードが使えたら助かります。でも、全く優先度は高くなく、見た目も見にくくなりそうなので、それが大変な作業だったら今回は含めなくてもOKです。

対応が可能だった場合はデザインもそれに対応する必要があるので言ってください〜

@mousu-a
Copy link
Contributor Author

mousu-a commented Nov 29, 2024

@machida
ありがとうございます!!
調査してみます🙏

@mousu-a
Copy link
Contributor Author

mousu-a commented Dec 5, 2024

@machida
liの中でリンクカードを展開できるようにする件ですが、こちら結構大変そうなことが判明したので、このまま(マークダウンのネストを許可するのはdetailsとmessageのみで)進めさせてもらえたらと思います🙇‍♂️

@mousu-a mousu-a force-pushed the feature/introduce-of-link-card branch from c692d79 to 803b1e9 Compare January 19, 2025 06:22

module LinkCard
class CardTest < ActiveSupport::TestCase
test '#metadata' do
Copy link
Contributor Author

@mousu-a mousu-a Jan 21, 2025

Choose a reason for hiding this comment

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

このテストでリンクカードの動きをまとめてテストしています。
(LinkCard::Ogpクラス、LinkCard::Cardクラス、Metadataクラス)

const sibling = targetLink.previousElementSibling
if (sibling.tagName === 'P') {
sibling.insertAdjacentElement('afterend', targetLink)
sibling.remove()
Copy link
Contributor Author

@mousu-a mousu-a Jan 22, 2025

Choose a reason for hiding this comment

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

リンクカードの元となったリンクを削除しています。
(Markdown-itはリンクが<p><a></a></p>の形で出力されるため、リンクの親要素であるP要素を削除しています)

)
}

const loadTwitterScript = () => {
Copy link
Contributor Author

@mousu-a mousu-a Jan 24, 2025

Choose a reason for hiding this comment

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

この関数でTweetのデザインを適用しています。(最終的なデザインはbootcampアプリ内で整えています)

JSでツイートを埋め込むときのベストプラクティス

@mousu-a
Copy link
Contributor Author

mousu-a commented Jan 24, 2025

@ayu-0505
お疲れ様です!
よろしければこちらのレビューをお願いできますでしょうか🙏
全く急ぎではないです。コード量が多く大変かもしれませんがご検討いただけますと幸いです🙇‍♂️

都合が合わなければ遠慮なく仰ってください〜🙏

@mousu-a mousu-a marked this pull request as ready for review January 24, 2025 04:10
@ayu-0505
Copy link
Contributor

@mousu-a
お疲れ様です🍵
レビュー了解しました!
実は現在別のレビューを1件受け持っておりまして(フィヨルドチョイスの不具合で表示されていないようです※)、少しお時間頂戴したいと思います。
具体的には1週間+α程度はお待ちいただければと思っております🙏
お急ぎではない、とのことなのですが、もし上記の内容で問題ありましたらまたご連絡いただけたらと思います🙇🏻‍♀️

※フィヨルドチョイス不具合について
いくつかのレビューissueが登録されない不具合があるようです。
以下のように「バグ報告スレッド」にはsugiweさんが報告してくださっています。
https://discord.com/channels/715806612824260640/828456619028381726/1331907039550767234

@mousu-a
Copy link
Contributor Author

mousu-a commented Jan 27, 2025

@ayu-0505
返信ありがとうございます!!

※フィヨルドチョイス不具合について
いくつかのレビューissueが登録されない不具合があるようです。

これは初耳でした👀

実は現在別のレビューを1件受け持っておりまして(フィヨルドチョイスの不具合で表示されていないようです※)、少しお時間頂戴したいと思います。
具体的には1週間+α程度はお待ちいただければと思っております🙏

承知いたしました!全然急ぎではないのでのんびりやっていただければと思います🍵
お忙しいところすみませんがよろしくお願いいたします🙇‍♂️


レビューに際して質問などあればドシドシお気軽にお願いいたします🙆‍♂️

@ayu-0505 ayu-0505 self-requested a review January 28, 2025 00:45
const embedToTweet = async (targetLink, url) => {
try {
const response = await fetch(
`/api/metadata?url=${encodeURIComponent(url)}&tweet=1`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

初めはtweet=trueで送っていたんですが、paramsで送ると絶対に文字列になってしまうので、受け取るときは"true"となってしまいます。
動きとしては別にどちらでも問題はないんですが、勘違いを避けるために"フラグが立っている状態"という意味で"1"を送っています。

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.

3 participants