Skip to content

Markdownの中でfigureタグを使えるようにした #8545

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

Merged
merged 14 commits into from
Jun 6, 2025

Conversation

sugiwe
Copy link
Contributor

@sugiwe sugiwe commented Apr 9, 2025

Issue

概要

Markdown記法の追加機能としてfigureタグを使えるようにしました。

:::figure
<a href="https://example.com/image.jpg"><img src="https://example.com/image.jpg"></a>
キャプション本文
:::

上記のように書くと、

<figure>
<a href="https://example.com/image.jpg"><img src="https://example.com/image.jpg"></a>
<figcaption>キャプション本文</figcaption>
</figure>

というHTMLに変換されます。

変更確認方法

  1. {feature/ebable-to-use-figure-tag-in-markdown}をローカルに取り込む
    • git fetch origin pull/8545/head:feature/ebable-to-use-figure-tag-in-markdown
    • git switch feature/ebable-to-use-figure-tag-in-markdown
  2. 今回用に新しく取り入れるnpmがあるのでbin/setupでセットアップする
  3. foreman start -f Procfile.devでローカルサーバを立ち上げる
  4. 任意のユーザーでログインし、日報作成ページ等のテキスト入力欄に下記をコピー&ペーストする
:::figure
<a href="https://example.com/image.jpg"><img src="https://example.com/image.jpg"></a>
キャプション本文
:::
  1. DevTools等を使い、プレビュー欄側で下記のようにHTMLが出力されていることを確認する
  • figureブロック全体がfigureタグで囲まれていること
  • キャプション部分がfigcaptionタグで囲まれていること

※上記のサンプルコードでもfigureタグ・figcaptionタグは確認できますが画像は表示されないので、任意の画像をアップロードしていただくと画像の表示も含めて確認が可能です。

Screenshot

変更前

1_before

変更後

2_after

@sugiwe
Copy link
Contributor Author

sugiwe commented Apr 9, 2025

@machida
お疲れ様です!
こちらの件、figureタグ及びfigcaptionタグで囲うことができるようになりましたので、デザインの追加をお願いできますでしょうか🙏
どうぞよろしくお願いいたします。

@machida
Copy link
Member

machida commented Apr 9, 2025

@sugiwe デザイン承知しました👍実装ありがとうございます🙏

@machida machida force-pushed the feature/ebable-to-use-figure-tag-in-markdown branch from 763a231 to 15b8fae Compare April 14, 2025 06:53
@machida
Copy link
Member

machida commented Apr 14, 2025

@sugiwe
お待たせしました🙇‍♂️
デザイン入れましたー

最新のmainブランチを取り込んだので、git pull --rebase origin main をお願いします。

@sugiwe
Copy link
Contributor Author

sugiwe commented Apr 15, 2025

@machida
デザイン反映ありがとうございます!手元で無事確認できました✨

@sugiwe
Copy link
Contributor Author

sugiwe commented Apr 15, 2025

@mousu-a
お疲れ様です☕️

こちらのレビューをお願いしたいのですが、可能でしょうか?
markdown追加機能のPRでして、リンクカード実装をご対応されているmousuさんに見ていただけたら嬉しいなと思いご連絡いたしました。

急いではおりませんので1〜2週間くらいでご対応いただけたら大変嬉しいですが、厳しいようでしたら遠慮なくおっしゃってください!

どうぞよろしくお願いいたします🙏

@sugiwe sugiwe requested a review from mousu-a April 15, 2025 05:48
@machida
Copy link
Member

machida commented Apr 15, 2025

@sugiwe 確認ありがとうございますー🙏

@sugiwe sugiwe marked this pull request as ready for review April 15, 2025 05:48
@mousu-a
Copy link
Contributor

mousu-a commented Apr 15, 2025

@sugiwe
レビュー依頼ありがとうございます!!
ぜひ引き受けさせていただきますー🙌

余談ですがsugiweさんの文章はいつも相手への気遣いが見えてこちらとしてはとても嬉しいです😊

一週間ほどを目安にレビューさせていただきますのでしばしお待ちください🙏

@sugiwe
Copy link
Contributor Author

sugiwe commented Apr 15, 2025

@mousu-a
レビュー快諾いただきありがとうございます!
(そしてなんと、とても嬉しい言葉もいただきありがとうございます✨)
どうぞよろしくお願いいたします🙏

Copy link
Contributor

@mousu-a mousu-a left a comment

Choose a reason for hiding this comment

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

@sugiwe
お疲れ様です🍵
レビューお待たせいたしました!(一週間はだいぶ嘘ついてしまいました😅)
いくつかコメントさせていただいていますのでご確認ください🙏

余談です

今回の実装は、renderer(のカスタマイズ)とruleの追加(プラグイン)を2つ使って実装しているようだったので、パッと見た時に「どうにかどちらか1つで出来ないかな〜」とあれこれやってみましたが中々難しいですね🤔

rule(プラグイン)であればrule1つで出来そうですけど、:::figure:::記法の捕捉はrendererを使った方が収まりが良さそうですし、かといってrenderer1つではどうやら実装は難しそうでした。(既存のmessage記法やdetails記法とはちょっと勝手が違って)

結果的にこの実装(renderer(のカスタマイズ)とrule(プラグイン)の二刀流)がベストとなったわけですね。(身をもって体験しました😅)

isInContainerFigure = true
figureIndexes.push(i)
}
if (isInContainerFigure && token.type === 'inline') {
Copy link
Contributor

@mousu-a mousu-a Apr 16, 2025

Choose a reason for hiding this comment

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

ここをこんな感じにするとremoveFigureParagraphしなくても良いかもです👀

if (isInContainerFigure && token.type === 'inline') {
        const linkedImageMatch = token.content.match(
          /<a [^>]+>\s*<img [^>]+>\s*<\/a>/
        )
        const linkedImageTag = linkedImageMatch ? linkedImageMatch[0] : ''
        const caption = token.content.replace(linkedImageTag, '').trim()
        token.content = `${linkedImageTag}${`<figcaption>${caption}</figcaption>`}`

        // markdown-itが自動出力してしまうfigureタグ内のpタグを出力しないようにする
        const pOpen = state.tokens[i - 1]
        const pClose = state.tokens[i + 1]
        pOpen.hidden = true
        pClose.hidden = true
      }

https://markdown-it.github.io/markdown-it/#Token.prototype.hidden

Copy link
Contributor Author

@sugiwe sugiwe Apr 17, 2025

Choose a reason for hiding this comment

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

こちらありがとうございます!
確かにToken#hiddenを使うとシンプルになりますね、というかこうすればrenderer無しで実装できるのですね…!
とても勉強になりました、こちら取り入れさせていただきます🙏

const buildFigureContent = (md) => {
md.core.ruler.after('block', 'extracting_caption_from_figure', (state) => {
let isInContainerFigure = false
const figureIndexes = []
Copy link
Contributor

Choose a reason for hiding this comment

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

使われていなさそうなので何かの消し忘れかも?figureIndexes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ありがとうございます、消し忘れでした🙏

/<a [^>]+>\s*<img [^>]+>\s*<\/a>/
)
const linkedImageTag = linkedImageMatch ? linkedImageMatch[0] : ''
const caption = token.content.replace(linkedImageTag, '').trim()
Copy link
Contributor

@mousu-a mousu-a Apr 16, 2025

Choose a reason for hiding this comment

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

提案です!
ここなんですが、
linkedImageTag = match[0]
caption = match[1]
といった感じで書けたらコードがもっと読みやすくなると思うのですがどうでしょうか?(token.contentに対してaタグ部分とキャプション部分をそれぞれ正規表現でグルーピングして取り出す感じです)

token.content.replace(linkedImageTag, '')の部分が、本来のreplace関数の意味するところの"置き換える"という意味で使っていないのでちょっとテクニカルかも?🤔

Copy link
Contributor

@mousu-a mousu-a Apr 16, 2025

Choose a reason for hiding this comment

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

ここの処理に関係することなんですが意図していなさそうな挙動を発見しました🔍


どうやらaタグとキャプション(テキスト)の間に空行が入っていると余計なfigcaptionタグが発生してしまうようです。

キャプチャ

スクリーンショット_2025-04-16_12_53_50-2

理由

おそらく、空行が入ることでtokenの配列に空行の文ズレが生じ、aタグのtokenとキャプション(テキスト)のtokenが別々に分かれてしまうことで発生していると思われます。

イメージとしては、本来意図している挙動はtoken.content #=> "<a>...</a>\nキャプション"なんですが、間に空行が入ると↓のようにtokenがバラバラになってしまう感じです。
aタグのtoken.content #=> "<a>...</a>"
空行のtoken.content #=> "\n"
キャプションのtoken.content #=> "キャプション"
tokentoken.contentindexをログに出して見てみるとわかりやすいかもです。


「aタグとキャプション(テキスト)の間に空行が入っていても同じように対応するかどうか」など、お手数ですがmachidaさんに一度ご確認いただけますでしょうか🙏
ただどちらにせよ、余分なタグが発生しないように調整する必要はありそうです💦

@sugiwe
Copy link
Contributor Author

sugiwe commented Apr 16, 2025

@mousu-a
お疲れ様です!
とても迅速にレビューしていただきありがとうございます、感謝です✨

余談のところもありがとうございます。
そうなんです、僕も初めもっとシンプルにできないものかと色々試してみたのですが、message記法やdetails記法と異なり、:::で挟まれるブロックの中にhtmlタグが含まれている関係で(?)今回のfigureはちょっと事情が異なることがわかり、一旦現在の状態に落ち着いたという感じです😅

それ以外で諸々コメントいただいた部分に関しまして、逆に僕の方が1週間くらいかかってしまうかもですが汗、詳細確認してまた修正・お返事させていただきます🙏

@mousu-a
Copy link
Contributor

mousu-a commented Apr 16, 2025

@sugiwe
お忙しい中お返事ありがとうございます😄
こちらも全然急いでないので大丈夫ですよ〜🙆‍♂️
のんびりお待ちしております🍵

@sugiwe sugiwe force-pushed the feature/ebable-to-use-figure-tag-in-markdown branch from 7436daf to 5d09fa6 Compare April 18, 2025 03:33
@mousu-a
Copy link
Contributor

mousu-a commented Apr 18, 2025

@sugiwe
再レビュー前にすみません🙏
ここなんですが、if(!match) returnとしておけば三項演算子で存在確認をしなくても良くなるかもです!(すみませんレビュー漏れでした〜💦)

(あとすみません、提案させていただいたコード内のmatchという変数名はあくまでも例なので変数名はご自由に変えてもらって大丈夫ですので🙇‍♂️)

@komagata
Copy link
Member

@sugiwe 途中で申し訳ないのですが下記のプラグインは使えないですかね。

https://github.com/rbottomley/markdown-it-figure

@sugiwe
Copy link
Contributor Author

sugiwe commented Apr 24, 2025

@komagata
お疲れ様です、ご連絡ありがとうございます!

共有していただいたmarkdown-it-figureですが、自分としては今回使うにはフィットしていないのではと感じました。

大きな理由は、markdown-it-figureが想定している入力方法が#[Caption](/url/to/image.png)のような形であるということです。

現在のbootcampアプリの仕様では、画像をアップロードすると自動的にimgタグをaタグで囲んだ状態、例えば以下のような状態で入力欄に入ります。

<a href="画像URL"><img src="画像URL" width="" height="" alt="ファイル名"></a>

これを、markdown-it-figureが想定している入力方法の#[Caption](/url/to/image.png)のような形になるように手作業で打ち込み直すとか、自動処理でそうなるようなカスタマイズを加えていくのはあまりシンプルではないと思いました。

それよりは、元々の画像アップロードの仕様として入力される<a href="画像URL"><img src="画像URL" width="" height="" alt="ファイル名"></a>::: figureのブロックで囲むという方法のほうがシンプルで良さそうかなと思ったのですが、いかがでしょうか?

@komagata
Copy link
Member

@sugiwe なるほどですね。発案者の @machida さんにも聞いてみたいと思います。

@machida こちらいかがでしょうか。

僕の考えとしては、

markdown-it-figureというデファクトなnpmがあるのであればそちらにFBCの仕様を合わせた方が今後のメンテナンスや他のタグでの実装など便利になるように思います。

今回の実装がmarkdown-it-figureに合わないのであればmarkdown-it-figureとの違い・メリットがわかるような名前としてmakrdown-it-xxxxxというプラグインとして実装するべきかなと思いました。

そして今後も新しいタグを実装する場合はmarkdown-itプラグインとして実装するという包括的な仕組みができると思います。

@sugiwe
Copy link
Contributor Author

sugiwe commented Apr 24, 2025

@komagata
早速お返事ありがとうございます!

補足としましては、今回::: figure 〜〜〜 :::という形で囲むということで、同様に::: で囲む形式としてbootcampアプリ内で実装済みのmarkdown-it-container-message.jsmarkdown-it-container-details.jsなどと一貫性を持たせる意図でmarkdown-it-container-figure.jsという名前で実装を進めております。
:::で囲んでいる実装ということでcontainerをつけており、結果的にmarkdown-it-figureとの違いも表現できているかなと思いました)

@machida
お手数ですがご確認の程よろしくお願いいたします🙏

@machida
Copy link
Member

machida commented Apr 24, 2025

@sugiwe @komagata
npmの方を確認しました!
npmの仕様だと画像の表示サイズを変更できないので、ブログ記事などを書く際は現状のようにHTMLを手書きすることを継続することになってしまいます。
なので、今のsugiweさんが実装している方で進めたいです。

@sugiwe
Copy link
Contributor Author

sugiwe commented Apr 27, 2025

@machida
ご確認ありがとうございます!
それでは駒形さんのお返事を待った上で現状のままで進行していければと思います🙏

@sugiwe
Copy link
Contributor Author

sugiwe commented Apr 27, 2025

@komagata
お疲れ様です、町田さんからご回答いただいた方針で、現状の実装のまま進めるということでよろしいでしょうか。
大丈夫そうであればメンバーレビューを再開できればと思いますので、お手数ですがお返事いただければ幸いです🙏

@komagata
Copy link
Member

@machida (CC: @sugiwe ) なるほどです。
markdown-itのプラグインとして作成し、npmとして作っていただきたいと思っているのですがそちらはいかがでしょうか?

@machida
Copy link
Member

machida commented Apr 30, 2025

@komagata それに関して僕としては特に意見はないですー

@sugiwe
Copy link
Contributor Author

sugiwe commented Apr 30, 2025

@komagata
お返事ありがとうございます!
「markdown-itのプラグインとして作成し、npmとして作る」というのは、僕のnpmアカウントで公開するということでよろしいでしょうか?
良い機会なのでチャレンジできればと思いますが、その場合、issueのポイントを2ポイントほどアップしていただきたいのですがいかがでしょうか?

@sugiwe
Copy link
Contributor Author

sugiwe commented Apr 30, 2025

@komagata
すみません、元々2ポイントなので2を足すと4になってしまいますね。4ポイントは無いと思うので3か5になると思いますが、できれば5ポイントにしていただきたいのですがいかがでしょうか?
理由として、元々調査と実装を進めるにあたりかなり時間を要してしまい、2ポイントから3ポイントに変更する相談をしようか迷っていたという背景があります。そこにnpm化の話も加わったので、さらにプラス2で5ポイントにできないかという相談になります。
全体のポイントバランスを踏まえたときに5にするのは難しいようでしたら何なりとおっしゃってください🙇🏻‍♂️

@komagata
Copy link
Member

komagata commented May 4, 2025

@sugiwe 僕としては元々npmを含めて2ぐらいで考えておりました。
他の皆さんがやっているIssueも実際にやってみると難易度も高いものがたくさんあり苦労しています。手間も時間もかかるものも多いです。
他と比較して考えると3ポイントぐらいじゃないかなと思っています。

仕事であれば会社の名義でnpmになりますが、FBCは練習なので @sugiwe さんのnpmとしていただいて大丈夫です。

@sugiwe sugiwe force-pushed the feature/ebable-to-use-figure-tag-in-markdown branch from 21ad71d to 450fb12 Compare May 8, 2025 20:34
@sugiwe
Copy link
Contributor Author

sugiwe commented May 13, 2025

@mousu-a
お疲れ様です☕️
時間が空いてしまいすみません、レビューしていただいた部分の対応およびnpm化して読み込む処理を反映いたしました。

npmは以下です!
https://www.npmjs.com/package/markdown-it-container-figure

お手数ですが改めてご確認をお願いできますでしょうか、よろしくお願いいたします🙏

@mousu-a
Copy link
Contributor

mousu-a commented May 13, 2025

@sugiwe
npm化お疲れ様です〜!
machidaさんへのご相談もありがとうございました☺️

いくつか気になる点を下にコメントさせていただきました📝
(npmになったのでこちらで失礼します🙏)

お手すきでご確認いただければと思います🙇‍♂️

気になる点

npmをインストールしてくる形に変えたので、変更確認方法にbin/setupが必要になりそうです😲


これはnpmの方になるのですが、testがないのが気になるな〜と思いました。
不特定多数の人が見る/使うことになるかもなので、testがあるとnpmのユーザーが安心できるかもです🤔
これはnpm化したことで発生したタスクになりますので、このissueの範囲に含めても良いのかなと思うのですがいかがでしょうか?


matchedImageAndCaptionという変数名についてですが、イメージとキャプションがどっちもないとマッチしないのかな?と思われちゃうかもしれません。
イメージのみの場合でもマッチするようになっていると思うので、変数名をそれに合わせてあげると良さそうに思います!

とここまで書いていて思ったのですが、画像をアップロードした際にaタグで囲むのはFBCの仕様なので、matchedImageAndCaptionの正規表現は現在FBCに特化した形になっていると思います。
npmとして公開するのであればさらに汎用的に、aタグに囲まれたimgタグよりも、imgタグにマッチする形が良いのかなと思いました。(正確にはimgタグとキャプションですね)

GitHubでもimgタグがaタグに囲まれて出力されますが、ZENNではpタグに囲まれて出力されるようです。
詳しい正規表現が思いついているわけではないので申し訳ないのですが、こちらご一考いただければと思います🙇‍♂️


(余談が関係なさすぎたのと質問は自己解決できたので削除しました〜見ちゃってたらごめんなさい🙏)


以上よろしくお願いします🙏

@sugiwe
Copy link
Contributor Author

sugiwe commented May 13, 2025

@mousu-a
お疲れ様です、上記ありがとうございます!
npm化にあたって考慮が必要なことを色々アドバイスいただき大変助かります✨
順番に検討・対応してまたご連絡いたしますので、少々お待ちくださいませ🙏(正規表現の部分が僕もひとまず頭の中では答えが出ていないので、また相談などさせていただくかもしれません…!)

(余談は見てなかったです!メールに来ている通知の方で見れはしますが😂 諸々書いていただきむしろありがたいです、ありがとうございます〜!)

@sugiwe
Copy link
Contributor Author

sugiwe commented May 23, 2025

@machida
お疲れ様です!先日は開発MTGにてありがとうございました。

以下の通り、figureブロック内に入れる画像について、対応する種類を広げることができました!

  • aタグに囲まれたimgタグ(元々、これだけに対応)
  • pタグに囲まれたimgタグ(追加で対応)
  • imgタグ単体(追加で対応)

※出力されるサンプルに尽きましてnpm側のREADMEを見ていただくのが良いかもなので、該当箇所のリンクを貼っておきます!
https://github.com/sugiwe/markdown-it-container-figure?tab=readme-ov-file#markdown-syntax

今回の対応に伴い、CSSの方も追加対応が必要になりました。

20250523a

上記のように、「pタグに囲まれたimgタグ」および「imgタグ単体」の場合に、画像とfigcaptionの間に少し広めのマージンができてしまっていますので、「aタグで囲まれたimgタグ」と同じマージンになるよう調整をお願いできますでしょうか?

お手数おかけしますがどうぞよろしくお願いいたします🙏

@mousu-a
Copy link
Contributor

mousu-a commented May 23, 2025

@sugiwe (CC: @machida)
横からすみません!
まずはsugiweさん、変更ありがとうございます🙇‍♂️
ちょっと気になったのでコメントさせていただければと思います🙏

今回の対応に伴い、CSSの方も追加対応が必要になりました。

こちらについてですが、自分としては、これはFBC内で使用するものなので、CSSでの対応は「aタグに囲まれたimgタグだけに対応する」、という形で良さそうに思っています。(正規表現をpタグに囲まれたimgタグ、imgタグ単体にもマッチするよう変更したのはとても良いと思います👍ありがとうございます!READMEもわかりやすかったです😄)

つまり、npmとして公開している部分(この場合は正規表現ですが)を汎用的にするのはとても良いと思うのですが、npmの仕様すべてにデザインを対応させる必要はなく、FBCで使う場合は、FBCの仕様のパターン(aタグに囲まれたimgタグ)のみデザインを対応させればいいんじゃないかな、と思ったのですがいかがでしょうか...!
判断はmachidaさんにお任せいたします🙏

判断材料になれば、と思いコメントさせていただきました🙇‍♂️

@sugiwe
Copy link
Contributor Author

sugiwe commented May 23, 2025

@mousu-a
コメントありがとうございます!

FBCアプリではデフォルトで画像アップと同時にaタグで囲まれるので現在のままでもおそらく問題はないのですが、aタグを取りimgタグ単体にしたりそれをpタグで挟む、という行為自体はできてしまうので、その場合の表示も整えると良いのかなと思った次第です。

なのですが、mousuさんのおっしゃる通り、あくまでFBCで使う範囲においてはデフォルトでついたaタグをわざわざ取ることはほぼ無いと考えられそうなので、machidaさん的に今のままで問題なさそうであれば、追加のCSS対応は無くても大丈夫です!

ちょっと悩んでいたところではあったのですが補足も何もなく依頼してしまっていたので、コメントいただけて大変感謝です🙏

@machida
Copy link
Member

machida commented May 23, 2025

@sugiwe figure周り、ちょっとSassのコードをいじるだけで汎用的にできたので更新しておきましたー

@sugiwe
Copy link
Contributor Author

sugiwe commented May 27, 2025

@machida
ご確認・ご対応いただきありがとうございますー!
遅くなってしまいましたが自分のローカルでも確認ができました◎

@mousu-a
僕の作業分でいうと前回からの変更は以下のnpm側のファイルになるので変則的な感じになってしまい大変恐縮なのですが、ご確認をお願いできますでしょうか🙏
https://github.com/sugiwe/markdown-it-container-figure
※差分は以下を見ていただくと良いかもです(もうマージはしちゃっていますが)
https://github.com/sugiwe/markdown-it-container-figure/pull/2/files

@mousu-a
Copy link
Contributor

mousu-a commented May 29, 2025

@sugiwe
お疲れ様です!テスト、変更確認方法などの対応ありがとうございます👍
READMEも良さそうに思います!

以下気になった点をコメントさせていただきましたのでご確認ください🙏


https://github.com/sugiwe/markdown-it-container-figure/blob/main/index.js#L32
これは何かのタグにネストしているかもしれないImgタグなので、それを名前に含めてあげた方がいいのかなって思いました!imageTagだとImgタグそのままを想像しちゃうかもですね。
しかし良い名前が思いついていません🤔imageContentsとか?
ちょっと一緒に考えてみてほしいです!


https://github.com/sugiwe/markdown-it-container-figure/blob/main/index.js#L27
matchという変数名は良いと思うんですが、何にマッチするのかがわからなくなっちゃった感じがありますね。正規表現を見ないといけなくなっちゃった感じです。(npmのREADMEやこのnpmの目的を考えれば一目瞭然ですが、コードだけで理解出来ると嬉しいです)

既存のMarkdown-it Pulginでは正規表現を定数に入れたりしているみたいです。
例:~~_RE〜〜Regex~~_REGEXなど
しかし良い名前が思いついていません🤔FIGURECONTENTS_REGEXとか?
これもあんまりしっくりきていないのでちょっと一緒に考えてみてほしいです!


https://github.com/sugiwe/markdown-it-container-figure/blob/main/index.js#L27
既存の実装ではmatchメソッドよりもexecメソッドを使ってるみたいです!
好みだとは思いますが、Markdown-itの作法に則る形の方が良いかも?

https://github.com/CenterForOpenScience/markdown-it-video/blob/030e8c9e96a0c9fd423ebf8ab93300adb4149013/index.js#L57
https://github.com/markdown-it/markdown-it/blob/master/lib/rules_core/smartquotes.mjs#L39
https://github.com/CenterForOpenScience/markdown-it-video/blob/HEAD/index.js#L57


https://github.com/sugiwe/markdown-it-container-figure/blob/main/index.js#L19
これは好みの問題だと思いますが、ruleの名前が普通の関数っぽい名前なのがちょっと気になりました!
既存のMarkdown-it Pulginのruleを見ていると、ruleっぽい名前にした方がいいかもです。
container-figureとかどうでしょうか。
これはお好みで良いと思います!対応しなくても問題ないです🙏


https://github.com/sugiwe/markdown-it-container-figure/blob/main/index.js#L26
if (!isInContainerFigure) return って形にして抜き出せば依存関係を分けられそうです!

if (!isInContainerFigure) return

if (token.type === 'inline' || token.type === 'html_block') {

ご確認のほどよろしくお願いいたします🙏

@sugiwe
Copy link
Contributor Author

sugiwe commented May 29, 2025

@mousu-a
諸々ありがとうございます!!!
コード修正する前にこちらにお返事させていただきます🙏

■ imageTagの名前

確かに、imageTagはimgタグそのものを指しているように見えそうですね…。
名前悩ましいのですが、最終的に${hoge}<figcaption>${caption}</figcaption>hogeに入ることを考えると、imageContentsも良さそうですし、具体的に書くならimageOrImageWrapperとかを考えたんですが、ちょっと冗長すぎる気がしています😅(シンプルにするならimageWrapperかなとも思ったんですがimage単体もあり得るのでちょっと違うかなと思いました)
他はimageHTMLとかも考えたのですがう〜〜んという感じで、imageContentsがちょうど良い頃合いのように思いました!

■ 正規表現の定数化

ありがとうございます、他の実装で確かに正規表現を定数に入れているのを確認しました👀
FIGURECONTENTS_REGEXはわかりやすくて良いと思いました、単語の区切りは付けてFIGURE_CONTENTS_REGEXでいかがでしょうか?

■ matchメソッド→execメソッド

他の実装なども色々みていただいて誠にありがとうございます🙏
こだわってmatchにしていたわけではなく他に合わせていくのが良いと思いましたので、execにしようと思います!

■ ruleの名前

こちらもありがとうございます、確かにこれもお作法に合わせていこうと思います🙏
container-figureにしようと思います!

■ 早期リターン

早期リターン使う方がわかりやすくなりそうですね、こちらも反映するようにいたします!


以上となります、ほぼmousuさんのお書きいただいた方針で修正できればと思っていますが、命名のところなど改めてご意見いただけますと幸いです🙇‍♂️

@mousu-a
Copy link
Contributor

mousu-a commented May 29, 2025

@sugiwe
お早い返信ありがとうございます!!
また変更を汲んでいただきありがとうございます🙏
sugiweさんの考えている通りに変更いただければと思います🙏


■ imageTagの名前

imageContentsがちょうど良い頃合いのように思いました!

汲んでいただきありがとうございます!提案もありがとうございました😄
では仰る通りにimageContentsでお願いします!
Wrapper良いですね〜思いつきませんでした👀)

■ 正規表現の定数化

単語の区切りは付けてFIGURE_CONTENTS_REGEXでいかがでしょうか?

区切りを忘れていました!
では仰る通りにFIGURE_CONTENTS_REGEXでお願いします!

■ matchメソッド→execメソッド

こだわってmatchにしていたわけではなく他に合わせていくのが良いと思いましたので、execにしようと思います!

👍

■ ruleの名前

container-figureにしようと思います!

👍
■ 早期リターン

こちらも反映するようにいたします!

👍

@sugiwe
Copy link
Contributor Author

sugiwe commented May 30, 2025

@mousu-a
お疲れ様です☕️
上記お返事ありがとうございます、ひと通りの修正を反映させました!

修正のPRは以下です。
sugiwe/markdown-it-container-figure#4

既にマージ済みでバージョンアップを本PRにも取り込みました、ご確認をお願いできますでしょうか🙏
どうぞよろしくお願いいたします!

@mousu-a
Copy link
Contributor

mousu-a commented May 31, 2025

@sugiwe
細かいところをコメントさせていただきました!
が修正必須ではないのでこちらからはApproveとさせていただきます〜!LGTM!

以下細かなコメントです!


https://github.com/sugiwe/markdown-it-container-figure/blob/main/index.js#L28
改行なくても良さそうかなって思います!


https://github.com/sugiwe/markdown-it-container-figure/blob/main/index.js#L36
imageCaptionとした方がimageContentsと合ってて良いかも?って思いました!


テストファイルの名前ってtest.jsじゃだめなのかな?って思いました!何かルールがあったりするんでしょうか?👀


以上長い間対応していただきありがとうございました!お疲れ様でした🎉

@mousu-a mousu-a self-requested a review May 31, 2025 01:13
@sugiwe
Copy link
Contributor Author

sugiwe commented May 31, 2025

@mousu-a
Approveありがとうございます!

改行は確かに意味的にその部分無くしたほうが良さそうです、ありがとうございます!
変数名もimageContentsと合わせるのはとても良さそうに思ったのでimageCaptionにしました!

以上2点は修正完了済みです🫡

テストファイル名ですが、test.jsでもテストを走らせる上で問題は無さそうですし現状はテストファイル対象が1つだけなので誤解が生じることは無いのですが、「index.jsのテストですよ」というを明示化するために個人的にはindex.test.jsのままにしておきたいなと思いました👀


こちらこそ、何度もレビューしていただき本当にありがとうございました!!!

@sugiwe
Copy link
Contributor Author

sugiwe commented Jun 1, 2025

@komagata
お疲れ様です、こちらメンバーレビューが終わりましたのでレビューをお願いできますでしょうか。

パッケージ化して取り込むようにしたnpmは以下です。
https://www.npmjs.com/package/markdown-it-container-figure

どうぞよろしくお願いいたします🙏

Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

確認させて頂きました。OKです〜🙆‍♂️

@komagata komagata merged commit 7278799 into main Jun 6, 2025
5 checks passed
@komagata komagata deleted the feature/ebable-to-use-figure-tag-in-markdown branch June 6, 2025 13:19
@github-actions github-actions bot mentioned this pull request Jun 6, 2025
15 tasks
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.

4 participants