Skip to content
This repository was archived by the owner on Aug 2, 2023. It is now read-only.

#8 PageListタグをMarkdownに書くと下位ページ一覧が表示されるように#34

Draft
book000 wants to merge 7 commits intomasterfrom
add-children-pages-list
Draft

#8 PageListタグをMarkdownに書くと下位ページ一覧が表示されるように#34
book000 wants to merge 7 commits intomasterfrom
add-children-pages-list

Conversation

@book000
Copy link
Copy Markdown
Member

@book000 book000 commented Apr 27, 2021

@book000 book000 added the ✨feature request 新機能や要望 label Apr 27, 2021
@book000 book000 requested a review from Hiratake April 27, 2021 16:27
Copy link
Copy Markdown
Member

@Hiratake Hiratake left a comment

Choose a reason for hiding this comment

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

PageList.vueglobal ディレクトリに入れている理由はなんですか?
特別な理由がないのであれば、他のコンポーネントと同様に components 直下にいれるべきだと思います。理由によっては、他のコンポーネントを global に移動、また global というディレクトリ名を変更するなどが必要になると思います。

Copy link
Copy Markdown
Member

@Hiratake Hiratake left a comment

Choose a reason for hiding this comment

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

content_ という Submodule が追加されているようですが、追加した理由はなんですか?
表示確認だけであれば既にある content 内の Markdown ファイルを変更で可能だと思います。理由によっては残しておくのもいいですが、 content_ という名前は用途が推測しやすいものに変更するべきだと思います。

@@ -0,0 +1,59 @@
<template>
<ul>
<li
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

リストの構造がおかしいです。
リストで階層を表現する場合は <li> の内側に <ul> タグを入れ、その内側に下層の項目を <li> でいれるのが普通だと思います。

</script>

<style lang="scss" scoped>
.page-list__item {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

階層のあるリストのCSSは components/ArticleBody.vue 内にあります。分からなければ参考にしてください。

@book000
Copy link
Copy Markdown
Member Author

book000 commented Apr 28, 2021

PageList.vueglobal ディレクトリに入れている理由はなんですか?
特別な理由がないのであれば、他のコンポーネントと同様に components 直下にいれるべきだと思います。理由によっては、他のコンポーネントを global に移動、また global というディレクトリ名を変更するなどが必要になると思います。

この作業に取り組みだした際、global ディレクトリに置くことでコンテンツ側からコンポーネントを呼び出せる…という話を見たことがあり、こういう配置にしたのですがローカル環境でglobalではないところに置いた際に普通に動作するので、確かに別ディレクトリに置く必要はないかもしれません。
次のコミットで修正します。

@book000
Copy link
Copy Markdown
Member Author

book000 commented Apr 28, 2021

content_ という Submodule が追加されているようですが、追加した理由はなんですか?
表示確認だけであれば既にある content 内の Markdown ファイルを変更で可能だと思います。理由によっては残しておくのもいいですが、 content_ という名前は用途が推測しやすいものに変更するべきだと思います。

これは完全にミスです。不要なディレクトリ(中身何もなし)なので、次のコミットで削除します。

@book000
Copy link
Copy Markdown
Member Author

book000 commented Apr 28, 2021

リストの構造

これは以前も一度質問したことがありますが…

20210428-152830-chrome-Ljypq7ZCif

nuxt/content が fetch() で返すデータは上の画像のようなデータで、階層構造によってデータが返ってくるわけではないため、非常にデータを調理しにくいです
よって components/ArticleToc.vue のように li の深さをクラスで指定することで疑似的に深さを表現するようにしたのですが、代替策はありますか?

スクリプト側でなんとかするとすれば、this.page をひとつづつ処理して item.path.split('/').length が変わったら階層を上げたり下げたりして配列を作成する…みたいな方法もないわけではないとは思いますが、非常に読みにくい処理となることは必至かと思います

@Hiratake
Copy link
Copy Markdown
Member

階層構造によってデータが返ってくるわけではないため、非常にデータを調理しにくい

たしかに、そのような会話があったような気がします。
以前の回答と異なるものになるかもしれませんが、どうしても不可能な場合を除き、誤った構造とするのは極力避けるべきだと思います。
今回のケースでは、複雑なコードとなるかもしれないということ以外に理由がないので対応すべきかと思いますが、いかがでしょうか?

処理が重くなるというのはあるかもしれませんが、事前にビルドしたページを公開しているため、サイトを閲覧する際に表示速度が遅くなるといったことはないと思います。

components/ArticleToc.vue のように

こちらの実装も誤った構造ということになるので、修正が必要ですね。

@book000
Copy link
Copy Markdown
Member Author

book000 commented Apr 29, 2021

たしかに、そのような会話があったような気がします。
以前の回答と異なるものになるかもしれませんが、どうしても不可能な場合を除き、誤った構造とするのは極力避けるべきだと思います。
今回のケースでは、複雑なコードとなるかもしれないということ以外に理由がないので対応すべきかと思いますが、いかがでしょうか?

処理が重くなるというのはあるかもしれませんが、事前にビルドしたページを公開しているため、サイトを閲覧する際に表示速度が遅くなるといったことはないと思います。
こちらの実装も誤った構造ということになるので、修正が必要ですね。

了解しました。

実装にトライしてはみますが、もしうまくいかなければ components/ArticleToc.vue の修正を待ってその修正を参考にしてこちらに取り入れるかもしれません。

どうしましょうか、一旦プルリクはクローズしたほうがいいですか?

@book000 book000 marked this pull request as draft December 14, 2021 16:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

✨feature request 新機能や要望

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants