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

feat: [zenn-markdown-html] VS Code 拡張機能でのスクロール同期のためのソースマップ追加 #504

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

senkenn
Copy link
Contributor

@senkenn senkenn commented Aug 7, 2024

📑 Summary

📋 Tasks

  • 📖 Contribution Guide を読んだ
  • 👩‍💻 canary ブランチに対するプルリクエストである
  • zenn-cli で実行して正しく動作しているか確認する
  • 不要なコードが含まれていないか( コメントやログの消し忘れに注意 )
  • XSS になるようなコードが含まれていないか
  • モバイル端末での表示が考慮されているか
  • Pull Request の内容は妥当か( 膨らみすぎてないか )

@senkenn senkenn changed the title feat: Add md-source-map utility for adding source map data attribute … feat: [zenn-markdown-html] VS Code 拡張機能のためのソースマップ機能追加 Aug 7, 2024
@senkenn senkenn changed the title feat: [zenn-markdown-html] VS Code 拡張機能のためのソースマップ機能追加 feat: [zenn-markdown-html] VS Code 拡張機能のためのソースマップ追加 Aug 7, 2024
@senkenn senkenn changed the title feat: [zenn-markdown-html] VS Code 拡張機能のためのソースマップ追加 feat: [zenn-markdown-html] VS Code 拡張機能でのスクロール同期のためのソースマップ追加 Aug 7, 2024
Copy link
Member

@cm-igarashi-ryosuke cm-igarashi-ryosuke left a comment

Choose a reason for hiding this comment

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

遅くなってすみません 🙏
内容確認しました!テストもしっかり書かれていてすばらしいです 🎉

小さなコメントをいくつかしましたので、ご確認をお願いいたします。

extensionの方はこれから確認します。

Comment on lines -66 to -69
test('リンク内のリンクを変換しない', () => {
const html = renderLink('- https://example.com\n- second');
expect(html).toEqual(
'<ul>\n<li><a href="https://example.com" target="_blank" rel="nofollow noopener noreferrer">https://example.com</a></li>\n<li>second</li>\n</ul>\n'

Choose a reason for hiding this comment

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

[memo] テストケースがL89と重複しているため削除。

Comment on lines 68 to 69
const iframe = parse(html).querySelector('span.zenn-embedded iframe');
expect(iframe).toBeNull();

Choose a reason for hiding this comment

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

iframeが存在しないことを確認するのはいい作戦と思いましたが、ここのテストケースの意図としては「details内のリンクが、リンクカードには変換されずリンクになる」ということを確認したいのだと思いました。

なので期待値としては以下を確認するようにしていただけますでしょうか?

  • aside iframe が存在しない
  • htmlに <a href="https://example.com" target="_blank" rel="nofollow noopener noreferrer">https://example.com</a> が含まれる

escapedClass !== '' ? `${escapedClass} code-line` : 'code-line'
}" ${
line !== undefined ? `data-line="${line}"` : ''
}>${content}</code></pre></div>`;

Choose a reason for hiding this comment

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

[q] このPRではなくていいのですが、highlightされたcontentにもdata-lineを設定することはできそうでしょうか?ある程度の高さがあるコードブロックだと、その部分もスクロール同期したいという要望が出てくるかなと思いまして。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

うーん申し訳ないですがなんとも言えないですね。ただ、VSCodeはそのような実装にはしていないようです。VSCodeはコードブロックの content には data-line を設けておらず、プレビュー側で調整しているようです。私もこのあたりはまだしっかりと把握できてはいないので間違っているかもしれません。

このあたりがそれっぽそうです。
https://github.com/microsoft/vscode/blob/89b2f90c3a903504fd324a00a45c52cfd6338738/extensions/markdown-language-features/preview-src/index.ts#L356-L372

今後、コードブロック以外にも画像の調整も必要になってきそうですね。

Choose a reason for hiding this comment

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

ご確認ありがとうございました!要望がたくさん上がってきたら検討することにしたいと思います。

This commit modifies the link rendering in the Zenn Markdown HTML tests. It replaces the usage of the 'span.zenn-embedded iframe' selector with 'aside iframe' to match the updated HTML structure. Additionally, it adds assertions to check that the rendered HTML contains the correct link format.

Co-authored-by: Ryosuke Igarashi <[email protected]>
Copy link
Contributor Author

@senkenn senkenn left a comment

Choose a reason for hiding this comment

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

修正と返信いたしました!

escapedClass !== '' ? `${escapedClass} code-line` : 'code-line'
}" ${
line !== undefined ? `data-line="${line}"` : ''
}>${content}</code></pre></div>`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

うーん申し訳ないですがなんとも言えないですね。ただ、VSCodeはそのような実装にはしていないようです。VSCodeはコードブロックの content には data-line を設けておらず、プレビュー側で調整しているようです。私もこのあたりはまだしっかりと把握できてはいないので間違っているかもしれません。

このあたりがそれっぽそうです。
https://github.com/microsoft/vscode/blob/89b2f90c3a903504fd324a00a45c52cfd6338738/extensions/markdown-language-features/preview-src/index.ts#L356-L372

今後、コードブロック以外にも画像の調整も必要になってきそうですね。

Copy link
Member

@cm-igarashi-ryosuke cm-igarashi-ryosuke left a comment

Choose a reason for hiding this comment

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

修正ありがとうございました!!!

LGTMですのでマージします!

@cm-igarashi-ryosuke cm-igarashi-ryosuke merged commit c85d438 into zenn-dev:canary Aug 15, 2024
3 checks passed
@senkenn senkenn deleted the feat/add-source-map branch August 17, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 機能追加・改善
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants