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.

Sign up for GitHub

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

Jump to bottom

【確認済】vk-blocksの読み込むファイルの役割のコメントを追加 #1386

Merged
merged 4 commits into from
Aug 24, 2022
Merged

【確認済】vk-blocksの読み込むファイルの役割のコメントを追加 #1386

merged 4 commits into from
Aug 24, 2022

Conversation

shimotmk
Copy link
Contributor

@shimotmk shimotmk commented Aug 23, 2022 •
edited
Loading

チケットへのリンク / 変更の理由（元のissueがあればリンクを貼り付ければOK）

vk-blocksの読み込むファイルの役割のコメントを追加

issueリンクを一応追加 #1382

どういう変更をしたか？

ファイルの役割のコメントを追加します。というか元々書いてあったものを戻します

このファイルはinc内にある各ロードするファイルを読み込むファイルだけ
https://github.com/vektor-inc/vk-blocks-pro/blame/4d85165e18fd8d50173334d2f1c9288e785bf70c/inc/vk-blocks-config.php

実装者の確認事項

実装者はレビュワーに回す前に以下の事を確認してチェックをつけてください。

  • 複数の意図の変更 （ 機能の不具合修正 + 別の機能追加など ） を含んでいないか？
  • Files changed (変更ファイル)の内容は目視で確認したか？
  • readme.txt に変更内容は書いたか？
  • 本当にちゃんと確認をしたか？

プログラムの変更の場合

テストを書かないのは普通ではありません。書けるテストは極力書くようにしてください。

  • 書けそうなテストは書いたか？

変更内容について何を確認したか、どういう方法で確認をしたかなど

コメント確認

確認URL

（　どこかのデモサイトかテストサーバーにデプロイ済みなどで確認できる場合はそのURL　）

レビュワーに回す前の確認事項

  • 実装者はこのテンプレートのチェック項目をちゃんと確認してチェックしたか？

レビュワー確認方法・確認内容など

コメントを見てファイルの役割がわかるかどうか

レビュワー向け

レビュワーが確認して変更が反映されていない場合の確認事項

レビューしてみて意図した動作をしない場合は再度ビルドするなど以下の項目を確認してください。

  • プルしたか？
  • ビルドしたか？
  • ビルドしたディレクトリは正しいか（別の開発環境のディレクトリを見ていないか）？
  • npm install したか？
  • composer install したか？
  • キャッシュをクリアして確認したか？

@shimotmk shimotmk marked this pull request as ready for review August 23, 2022 02:17
@drill-lancer
Copy link
Member

@shimotmk
/view/ のディレクトリの中身はサーバーサイドレンダリングのコールバックの補助関数のようなものなのでスタイルとは関係なさそうな気がします。

@shimotmk
fix / load
f33eb38 
require位置を調整
@shimotmk
Copy link
Contributor Author

@drill-lancer
ありがとうございます！
確かにコメントとあっていないですね
微調整しました

/view/class-vk-blocks-postlist.phpは複数のブロックで使われるのでutilsディレクトリに
/view/responsive-br.phpはextensions/commonディレクトリ
を作って移行しようかなと思っていました。
いかがでしょうか？

@drill-lancer
Copy link
Member

@shimotmk
/view/responsive-br.php も分割読み込み以外関係ないので別物なので私なら同じように下に移動します。
移動についてはそれで良いと思います。

@shimotmk
fix / load
6fafe85 
require位置を調整
@drill-lancer
Copy link
Member

@shimotmk
approve します

@shimotmk
Copy link
Contributor Author

@drill-lancer ありがとうございます！

mthaichi
mthaichi reviewed Aug 23, 2022
View reviewed changes
Copy link
Contributor

@mthaichi mthaichi left a comment

Choose a reason for hiding this comment

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

@shimotmk ありがとうございます！
僕なりの意見を書いてみましたので、ご意見お聞かせくださいー。

inc/vk-blocks/vk-blocks-functions.php
*
* Main Functions for VK Blocks
* このファイルはinc/vk-blocks内にあるフォルダ内を読み込むだけ
Copy link
Contributor

Choose a reason for hiding this comment

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

こちらはファイルの内容と合っていない感じがします。
元々、汎用的に利用する関数をここで定義するつもりだったと思うんですね。
なので、本来ならば「共通関数の定義」となるのかなと。

なんてことを書いているうちに思ったのですが、
冒頭のサーバーサイドレンダリングスクリプトの読み込みもvk-blocks-config.php でやったらいいと思うんですね。そうすると、こちらは関数の定義だけになり、役割分担がすっきりします。

vk-blocks-config.php も vk-blocks-pro.php からしか読み込まないのであれば、vk-blocks-load.php という名前に変えてもいい気もします。話広がってすみません。ちょっと検討してみていただけますと幸いです。

inc/vk-blocks-config.php Outdated
* Load VK Blocks Files
* Load VK Blocks Inc Files
*
* このファイルはinc内にある各ロードするファイルを読み込むファイルだけ
Copy link
Contributor

Choose a reason for hiding this comment

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

「このファイルはinc内にある各ロードするファイルを読み込むファイルだけ」

ちょっと日本語がおかしいかな？？？
「各種ファイルの読み込み」ぐらいで良いかなと思いました。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

これの通りですが、このファイルでロードするのはincのフォルダ数+inc直下にあるファイル数(現在は13)にしたいです
#1386 (comment)

@mthaichi mthaichi changed the title 【確認待ち】vk-blocksの読み込むファイルの役割のコメントを追加 【2人目確認中】vk-blocksの読み込むファイルの役割のコメントを追加 Aug 23, 2022
@shimotmk
Copy link
Contributor Author

@mthaichi
ご意見ありがとうございます！

まず、このブランチはロードするファイルに役割を書くことが目的です。
ファイル名の変更などは一旦したく無いです
(それは別の問題なのですが無料版へのデプロイがまだ正常になっていないからです #1381 )

そして
inc/tgm-plugin-activation
inc/vk-admin
…
などはvektor-wp-librariesで管理されているライブラリです。

vk-blocksが管理しているのはinc/vk-blocks内のフォルダです
(inc/font-awesomeなどの例外はありますが、そうしたいです。なので今後直します
というか元々そうだったぽいです)

そうなるとinc/vk-blocks-config.phpでvk-blocks内のそれぞれのファイルをロードするのは違うのかなと思います

そして最終的にはjsのsrcフォルダみたいなものがinc/vk-blocksの中のようになったらわかりやすいかと思っています。

一旦このブランチではファイル名のことは忘れてみた方が良いかと思います

@mthaichi
Copy link
Contributor

@shimotmk すみません。いったんリネーム云々は忘れてください。僕もこのブランチで対応してほしいというつもりで書いたわけではありません 🙏

vk-blocksが管理しているのはinc/vk-blocks内のフォルダです
(inc/font-awesomeなどの例外はありますが、そうしたいです。なので今後直します
というか元々そうだったぽいです)

そうなるとinc/vk-blocks-config.phpでvk-blocks内のそれぞれのファイルをロードするのは違うのかなと思います

これは理解しました。

ただ、「このファイルはinc内にある各ロードするファイルを読み込むファイルだけ」というコメントは僕は何度読んでも理解ができません。（元々あったコメントで下村さんが書いたわけじゃないことはわかっています）
たとえば「inc 直下にあるローディングスクリプトのみを読み込む」 であれば理解できますが、そういう感じではだめですか？

vk-blocks-functions.php の冒頭のコメントはファイル全体の役割であるべきだと思うので、やはりちょっと違う気がししまいます。 「サーバーサイドレンダリングスクリプトを読み込み」というコメントがあるので、それで説明は足りるかなとも思いますし、冒頭にこのコメントを入れる意味を知りたいです。

しもむらさんの意図を十分理解していないがゆえかもしれません。すみません。

@shimotmk
Copy link
Contributor Author

@mthaichi

inc 直下にあるローディングスクリプトのみを読み込む

私の認識としては
ローディング：Webサイトの読み込み待ちのローディングアニメーションと誤解しそう
スクリプト：プログラム自体のこと。間違ってはいないがローディングスクリプトと書くとアニメーションのプログラムと誤解しそうだなと思います

要するに「inc直下にある」を入れたいということですよね？

このファイルはinc 直下にある各ロードするファイルを読み込むだけ

に変更しておきました

「サーバーサイドレンダリングスクリプトを読み込み」から文言を変えた理由はサーバーサイドレンダリングスクリプトというものから連想する範囲が広いと思ったからです。

私の認識としてはファイル全体のコメントは

<?php
/**
 *
 * @package vk_blocks
 */

の間だと思っています

@mthaichi
Copy link
Contributor

mthaichi commented Aug 24, 2022 •
edited
Loading

このファイルはinc 直下にある各ロードするファイルを読み込むだけ

に変更しておきました

これについてはOKだと思います。
「直下」云々より、元々のコメントは日本語としておかしいので、わかりづらいということです。
「このファイルはinc内にある各ロードするファイルのみを読み込む」だったらまだ理解できました。
直下を入れることで、明確なのでより良いと思います！

「サーバーサイドレンダリングスクリプトを読み込み」から文言を変えた理由はサーバーサイドレンダリングスクリプトというものから連想する範囲が広いと思ったからです。

私の認識としてはファイル全体のコメントは

私は以下のコメントの
「 Load VK Blocks Inc Files」と「 このファイルはinc直下にある各ロードするファイルを読み込むだけ」の指摘をしています。

<?php
/**
 * Load VK Blocks Inc Files
 *
 * このファイルはinc直下にある各ロードするファイルを読み込むだけ
 *
 * @package vk-blocks
 */

ここはファイル全体のコメントになりませんか。
その下の「// オプション値などでスタイルを作る処理を読み込み.」は良いと思います。
このファイル自体、後半は関数の定義がメインになっているので、名が体を表しきっていないのかなと単純に思った次第です。
ここに定義されている関数は後々引っ越しするんだということであれば、納得です。

細かいことですみません。細かい調整ありがとうございます。

@mthaichi
Copy link
Contributor

mthaichi commented Aug 24, 2022 •
edited
Loading

@shimotmk 「私ならこうする」を書いておきます。

Load VK Blocks Inc Files
VK Blocks main functions
 
inc直下にある各ロードするファイルの読み込み
共通関数の定義

@shimotmk
Copy link
Contributor Author

@mthaichi
単純にタイポでした

inc/vk-blocks-config.phpのことですか？？

inc/vk-blocks-config.phpは現状関数の定義は書かれて無いと思いますが、後々このファイルはinc直下にある各ロードするファイルを読み込むだけにする予定です

ここにも書きましたがinc/vk-blocks-config.phpでロードするのはincのフォルダ数+inc直下にあるファイル数(現在は13)にしたいです
#1386 (comment)

inc/vk-blocks/vk-blocks-functions.phpのことでしたらsrcディレクトリのようにutilsを作って移動する予定です
#1386 (comment)

@mthaichi
Copy link
Contributor

inc/vk-blocks/vk-blocks-functions.phpのことでしたらsrcディレクトリのようにutilsを作って移動する予定です
#1386 (comment)

@shimotmk 僕のどっかでtypoしたかもしれません。すみません。functionの方です。
上記の説明で納得です。OKだと思います。

@mthaichi mthaichi merged commit bb29932 into develop Aug 24, 2022
@mthaichi mthaichi deleted the doc/vk-blocks-load branch August 24, 2022 01:35
@shimotmk shimotmk mentioned this pull request Aug 24, 2022
Closed
14 tasks
@mthaichi mthaichi changed the title 【2人目確認中】vk-blocksの読み込むファイルの役割のコメントを追加 【確認済】vk-blocksの読み込むファイルの役割のコメントを追加 Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mthaichi mthaichi mthaichi left review comments

@drill-lancer drill-lancer drill-lancer approved these changes

@sysbird sysbird Awaiting requested review from sysbird

@osmdik osmdik Awaiting requested review from osmdik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shimotmk @drill-lancer @mthaichi