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

ニュースページの追加 #196

Merged
merged 8 commits into from
Jun 24, 2024

Conversation

jdkfx
Copy link
Contributor

@jdkfx jdkfx commented Apr 3, 2024

内容

  • ニュース記事の一覧を表示するページを追加
  • ニュース記事の個別ページを表示する機能を追加

関連 Issue

ref #97

スクリーンショット・動画など

image1
image2

その他

voicevox_blog/src/markdownsに、newsというディレクトリを作成し、その中でマークダウンファイルを作成するとニュース一覧ページに記事のリンクがタイトルと共に展開されます。

サンプルで用いた記事のマークダウンはこちらです。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

おー!!!良いですね!!!

gatsbyやGraphQLのあれこれが大変かとは思うのですが、できたら他の記事にも応用できると思うので、ぜひ・・・・!!!

@jdkfx jdkfx marked this pull request as ready for review June 17, 2024 14:05
@jdkfx jdkfx changed the title [WIP] ニュース一覧ページの追加 ニュースページの追加 Jun 17, 2024
@jdkfx
Copy link
Contributor Author

jdkfx commented Jun 17, 2024

残タスクとして残っていた個別記事ページの作成を行いました。

テストでnews/の中に二つマークダウンファイルを作成しています。どのように表示されるのかブランチを持っていってご確認していただいたあと、マージなどする前にこちらで削除します。

また。news/ディレクトリの中に記事が存在していないという前提で実装していなかったため、記事がない状態ですとエラーが起きてしまいます。よって、残タスクとして、記事がない状態でもエラーなどを出さないといった処理を実装することが挙げられます。

もしくは、既にあるニュース記事にしたい内容をこちらのPRの中に合わせて作成することも可能です。

@jdkfx jdkfx requested a review from Hiroshiba June 17, 2024 14:18
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

ほぼLGTMです!!

いくつかコメントさせていただきましたが、概ねとても良さそうに思いました!!
まだ記事はできていないので、表示上からは消しつつmainブランチにマージしたいです!
例えばヘッダーからのニュースページへのリンクをコメントアウトしつつ、Seoコンポーネントでnoindex={true} // TODO: リリース時に外すなどと書くのはどうでしょう・・・!

あ!もしご興味あればなのですが、ogp画像やmarkdown内で画像が使えるようになっていると非常に嬉しいかもです・・・!🙇
ちょっと調べた感じ、OGP画像はこんな感じ、markdown内画像はこんな感じでできるかも・・・・・・?
もちろん一度マージして後続のPRとかでも・・・!!!

gatsby-config.js Outdated Show resolved Hide resolved
gatsby-node.ts Outdated Show resolved Hide resolved
gatsby-node.ts Show resolved Hide resolved
to={"/news/"}
className="has-text-weight-bold is-underlined"
>
ニュース
Copy link
Member

Choose a reason for hiding this comment

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

変更履歴の左くらいに移動させていただくかもです!

src/pages/news/index.tsx Outdated Show resolved Hide resolved
@jdkfx
Copy link
Contributor Author

jdkfx commented Jun 23, 2024

レビューしていただきありがとうございます!!この後再度修正を行います!

例えばヘッダーからのニュースページへのリンクをコメントアウトしつつ、Seoコンポーネントでnoindex={true} // TODO: リリース時に外すなどと書くのはどうでしょう・・・!

かしこまりました!こちらもこの後修正を行っておきます!

ogp画像やmarkdown内で画像が使えるようになっていると非常に嬉しいかもです

こちらもやりたいのですが、今回のPR内容では盛り込みすぎるのではないかと感じたため、別のPRとして盛り込みたいです!

@Hiroshiba
Copy link
Member

ありがとうございます!!

こちらもやりたいのですが、今回のPR内容では盛り込みすぎるのではないかと感じたため、別のPRとして盛り込みたいです!

おお、検討してくださって嬉しいです!!
あっもちろん別のPRのがこちらとしてもありがたいです!!

@jdkfx
Copy link
Contributor Author

jdkfx commented Jun 24, 2024

@Hiroshiba
諸々の修正を行いました。再度レビューをお願いいたします!

@jdkfx jdkfx requested a review from Hiroshiba June 24, 2024 07:22
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

ちょっとこちらでフォーマッターにかけさせていただきます!

@jdkfx
Copy link
Contributor Author

jdkfx commented Jun 24, 2024

よろしくお願いします!🙇

@Hiroshiba
Copy link
Member

なぜかpushできなかったので、別PRでマージします!

@Hiroshiba Hiroshiba merged commit 230a2bf into VOICEVOX:master Jun 24, 2024
1 check passed
@Hiroshiba Hiroshiba mentioned this pull request Jun 24, 2024
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.

2 participants