Conversation
|
@machida |
|
@sugiwe デザイン承知しました👍実装ありがとうございます🙏 |
machida
force-pushed
the
feature/ebable-to-use-figure-tag-in-markdown
branch
from
763a231 to
15b8fae
Compare
|
@sugiwe 最新のmainブランチを取り込んだので、git pull --rebase origin main をお願いします。 |
|
@machida |
|
@mousu-a こちらのレビューをお願いしたいのですが、可能でしょうか？ 急いではおりませんので1〜2週間くらいでご対応いただけたら大変嬉しいですが、厳しいようでしたら遠慮なくおっしゃってください！ どうぞよろしくお願いいたします🙏 |
|
@sugiwe 確認ありがとうございますー🙏 |
|
@sugiwe 余談ですがsugiweさんの文章はいつも相手への気遣いが見えてこちらとしてはとても嬉しいです😊 一週間ほどを目安にレビューさせていただきますのでしばしお待ちください🙏 |
|
@mousu-a |
There was a problem hiding this comment.
@sugiwe
お疲れ様です🍵
レビューお待たせいたしました！（一週間はだいぶ嘘ついてしまいました😅）
いくつかコメントさせていただいていますのでご確認ください🙏
余談です
今回の実装は、renderer(のカスタマイズ)とruleの追加(プラグイン)を2つ使って実装しているようだったので、パッと見た時に「どうにかどちらか1つで出来ないかな〜」とあれこれやってみましたが中々難しいですね🤔
rule(プラグイン)であればrule1つで出来そうですけど、:::figure:::記法の捕捉はrendererを使った方が収まりが良さそうですし、かといってrenderer1つではどうやら実装は難しそうでした。(既存のmessage記法やdetails記法とはちょっと勝手が違って)
結果的にこの実装(renderer(のカスタマイズ)とrule(プラグイン)の二刀流)がベストとなったわけですね。（身をもって体験しました😅）
| isInContainerFigure = true | ||
| figureIndexes.push(i) | ||
| } | ||
| if (isInContainerFigure && token.type === 'inline') { |
There was a problem hiding this comment.
ここをこんな感じにすると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
There was a problem hiding this comment.
こちらありがとうございます！
確かにToken#hiddenを使うとシンプルになりますね、というかこうすればrenderer無しで実装できるのですね…！
とても勉強になりました、こちら取り入れさせていただきます🙏
| const buildFigureContent = (md) => { | ||
| md.core.ruler.after('block', 'extracting_caption_from_figure', (state) => { | ||
| let isInContainerFigure = false | ||
| const figureIndexes = [] |
There was a problem hiding this comment.
使われていなさそうなので何かの消し忘れかも？figureIndexes
| /<a [^>]+>\s*<img [^>]+>\s*<\/a>/ | ||
| ) | ||
| const linkedImageTag = linkedImageMatch ? linkedImageMatch[0] : '' | ||
| const caption = token.content.replace(linkedImageTag, '').trim() |
There was a problem hiding this comment.
提案です！
ここなんですが、
linkedImageTag = match[0]
caption = match[1]
といった感じで書けたらコードがもっと読みやすくなると思うのですがどうでしょうか？（token.contentに対してaタグ部分とキャプション部分をそれぞれ正規表現でグルーピングして取り出す感じです）
token.content.replace(linkedImageTag, '')の部分が、本来のreplace関数の意味するところの"置き換える"という意味で使っていないのでちょっとテクニカルかも？🤔
There was a problem hiding this comment.
ここの処理に関係することなんですが意図していなさそうな挙動を発見しました🔍
どうやらaタグとキャプション(テキスト)の間に空行が入っていると余計なfigcaptionタグが発生してしまうようです。
キャプチャ
理由
おそらく、空行が入ることでtokenの配列に空行の文ズレが生じ、aタグのtokenとキャプション(テキスト)のtokenが別々に分かれてしまうことで発生していると思われます。
イメージとしては、本来意図している挙動はtoken.content #=> "<a>...</a>\nキャプション"なんですが、間に空行が入ると↓のようにtokenがバラバラになってしまう感じです。
aタグのtoken.content #=> "<a>...</a>"
空行のtoken.content #=> "\n"
キャプションのtoken.content #=> "キャプション"
tokenやtoken.content、indexをログに出して見てみるとわかりやすいかもです。
「aタグとキャプション(テキスト)の間に空行が入っていても同じように対応するかどうか」など、お手数ですがmachidaさんに一度ご確認いただけますでしょうか🙏
ただどちらにせよ、余分なタグが発生しないように調整する必要はありそうです💦
|
@mousu-a 余談のところもありがとうございます。 それ以外で諸々コメントいただいた部分に関しまして、逆に僕の方が1週間くらいかかってしまうかもですが汗、詳細確認してまた修正・お返事させていただきます🙏 |
|
@sugiwe |
sugiwe
force-pushed
the
feature/ebable-to-use-figure-tag-in-markdown
branch
from
7436daf to
5d09fa6
Compare
|
@sugiwe 途中で申し訳ないのですが下記のプラグインは使えないですかね。 |
|
@komagata 共有していただいたmarkdown-it-figureですが、自分としては今回使うにはフィットしていないのではと感じました。 大きな理由は、markdown-it-figureが想定している入力方法が 現在のbootcampアプリの仕様では、画像をアップロードすると自動的にimgタグをaタグで囲んだ状態、例えば以下のような状態で入力欄に入ります。
これを、markdown-it-figureが想定している入力方法の それよりは、元々の画像アップロードの仕様として入力される |
|
@sugiwe なるほどですね。発案者の @machida さんにも聞いてみたいと思います。 @machida こちらいかがでしょうか。 僕の考えとしては、
今回の実装が そして今後も新しいタグを実装する場合はmarkdown-itプラグインとして実装するという包括的な仕組みができると思います。 |
|
@komagata 補足としましては、今回 @machida |
|
@machida |
|
@komagata |
|
@komagata それに関して僕としては特に意見はないですー |
|
@komagata |
|
@komagata |
sugiwe
force-pushed
the
feature/ebable-to-use-figure-tag-in-markdown
branch
from
21ad71d to
450fb12
Compare
|
@mousu-a npmは以下です！ お手数ですが改めてご確認をお願いできますでしょうか、よろしくお願いいたします🙏 |
|
@sugiwe いくつか気になる点を下にコメントさせていただきました📝 お手すきでご確認いただければと思います🙇♂️ 気になる点npmをインストールしてくる形に変えたので、変更確認方法に これはnpmの方になるのですが、testがないのが気になるな〜と思いました。
とここまで書いていて思ったのですが、画像をアップロードした際にaタグで囲むのはFBCの仕様なので、 GitHubでもimgタグがaタグに囲まれて出力されますが、ZENNではpタグに囲まれて出力されるようです。 （余談が関係なさすぎたのと質問は自己解決できたので削除しました〜見ちゃってたらごめんなさい🙏） 以上よろしくお願いします🙏 |
|
@mousu-a （余談は見てなかったです！メールに来ている通知の方で見れはしますが😂 諸々書いていただきむしろありがたいです、ありがとうございます〜！） |
|
@machida 以下の通り、figureブロック内に入れる画像について、対応する種類を広げることができました！
※出力されるサンプルに尽きましてnpm側のREADMEを見ていただくのが良いかもなので、該当箇所のリンクを貼っておきます！ 今回の対応に伴い、CSSの方も追加対応が必要になりました。
上記のように、「pタグに囲まれたimgタグ」および「imgタグ単体」の場合に、画像とfigcaptionの間に少し広めのマージンができてしまっていますので、「aタグで囲まれたimgタグ」と同じマージンになるよう調整をお願いできますでしょうか？ お手数おかけしますがどうぞよろしくお願いいたします🙏 |
|
@sugiwe (CC: @machida)
こちらについてですが、自分としては、これはFBC内で使用するものなので、CSSでの対応は「aタグに囲まれたimgタグだけに対応する」、という形で良さそうに思っています。（正規表現をpタグに囲まれたimgタグ、imgタグ単体にもマッチするよう変更したのはとても良いと思います👍ありがとうございます！READMEもわかりやすかったです😄） つまり、npmとして公開している部分（この場合は正規表現ですが）を汎用的にするのはとても良いと思うのですが、npmの仕様すべてにデザインを対応させる必要はなく、FBCで使う場合は、FBCの仕様のパターン(aタグに囲まれたimgタグ)のみデザインを対応させればいいんじゃないかな、と思ったのですがいかがでしょうか...！ 判断材料になれば、と思いコメントさせていただきました🙇♂️ |
|
@mousu-a FBCアプリではデフォルトで画像アップと同時にaタグで囲まれるので現在のままでもおそらく問題はないのですが、aタグを取りimgタグ単体にしたりそれをpタグで挟む、という行為自体はできてしまうので、その場合の表示も整えると良いのかなと思った次第です。 なのですが、mousuさんのおっしゃる通り、あくまでFBCで使う範囲においてはデフォルトでついたaタグをわざわざ取ることはほぼ無いと考えられそうなので、machidaさん的に今のままで問題なさそうであれば、追加のCSS対応は無くても大丈夫です！ ちょっと悩んでいたところではあったのですが補足も何もなく依頼してしまっていたので、コメントいただけて大変感謝です🙏 |
|
@sugiwe figure周り、ちょっとSassのコードをいじるだけで汎用的にできたので更新しておきましたー |
|
@machida @mousu-a |
|
@sugiwe 以下気になった点をコメントさせていただきましたのでご確認ください🙏 https://github.com/sugiwe/markdown-it-container-figure/blob/main/index.js#L32 https://github.com/sugiwe/markdown-it-container-figure/blob/main/index.js#L27 既存のMarkdown-it Pulginでは正規表現を定数に入れたりしているみたいです。 https://github.com/sugiwe/markdown-it-container-figure/blob/main/index.js#L27
https://github.com/sugiwe/markdown-it-container-figure/blob/main/index.js#L19 https://github.com/sugiwe/markdown-it-container-figure/blob/main/index.js#L26 if (!isInContainerFigure) return
if (token.type === 'inline' || token.type === 'html_block') {ご確認のほどよろしくお願いいたします🙏 |
|
@mousu-a ■ imageTagの名前確かに、 ■ 正規表現の定数化ありがとうございます、他の実装で確かに正規表現を定数に入れているのを確認しました👀 ■ matchメソッド→execメソッド他の実装なども色々みていただいて誠にありがとうございます🙏 ■ ruleの名前こちらもありがとうございます、確かにこれもお作法に合わせていこうと思います🙏 ■ 早期リターン早期リターン使う方がわかりやすくなりそうですね、こちらも反映するようにいたします！ 以上となります、ほぼmousuさんのお書きいただいた方針で修正できればと思っていますが、命名のところなど改めてご意見いただけますと幸いです🙇♂️ |
|
@sugiwe ■ imageTagの名前
汲んでいただきありがとうございます！提案もありがとうございました😄 ■ 正規表現の定数化
区切りを忘れていました！ ■ matchメソッド→execメソッド
👍 ■ ruleの名前
👍
👍 |
|
@mousu-a 修正のPRは以下です。 既にマージ済みでバージョンアップを本PRにも取り込みました、ご確認をお願いできますでしょうか🙏 |
|
@sugiwe 以下細かなコメントです！ 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 テストファイルの名前ってtest.jsじゃだめなのかな？って思いました！何かルールがあったりするんでしょうか？👀 以上長い間対応していただきありがとうございました！お疲れ様でした🎉 |
|
@mousu-a 改行は確かに意味的にその部分無くしたほうが良さそうです、ありがとうございます！ 以上2点は修正完了済みです🫡 テストファイル名ですが、
こちらこそ、何度もレビューしていただき本当にありがとうございました！！！ |
|
@komagata パッケージ化して取り込むようにしたnpmは以下です。 どうぞよろしくお願いいたします🙏 |
4 participants
Issue
概要
Markdown記法の追加機能としてfigureタグを使えるようにしました。
上記のように書くと、
というHTMLに変換されます。
変更確認方法
git fetch origin pull/8545/head:feature/ebable-to-use-figure-tag-in-markdowngit switch feature/ebable-to-use-figure-tag-in-markdownbin/setupでセットアップするforeman start -f Procfile.devでローカルサーバを立ち上げる※上記のサンプルコードでもfigureタグ・figcaptionタグは確認できますが画像は表示されないので、任意の画像をアップロードしていただくと画像の表示も含めて確認が可能です。
Screenshot
変更前
変更後