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

[ Outer ] 区切りに大きい三角とギザギザを追加 #2096

Merged
merged 25 commits into from
Jul 16, 2024

Conversation

mtdkei
Copy link
Contributor

@mtdkei mtdkei commented Jul 5, 2024

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

#1997 のギザギザと大きい三角

どういう変更をしたか?

Outerの区切り線に大きい三角(Large triangle)ギザギザ(serrated)を追加しました。

スクリーンショットまたは動画

変更前 Before

image

変更後 After

編集画面
image
image

フロントエンド
image
image

実装者の確認事項

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

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

プログラムの変更の場合

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

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

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

  1. 編集画面で区切りの設定 > タイプ「大きい三角」「ギザギザ」を選択。
  2. マイナスを含む区切りの数値、また、デバイス毎の設定の設定を行い保存。
  3. フロントエンドで編集画面の状態が確認できました。
    なお、ギザギザはマイナスの時もプラスと同じ表示になります。

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

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

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

実装者と同じ。
また、開発の方はコードの確認をしてください。


レビュワー向け

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

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

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

@mtdkei mtdkei changed the title [ Outer ] 区切りにギザギザを追加 [ Outer ] 区切りに大きい三角とギザギザを追加 Jul 11, 2024
@mtdkei mtdkei changed the title [ Outer ] 区切りに大きい三角とギザギザを追加 【確認待ち】[ Outer ] 区切りに大きい三角とギザギザを追加 Jul 11, 2024
@mtdkei mtdkei marked this pull request as ready for review July 11, 2024 06:28
@mthaichi mthaichi changed the title 【確認待ち】[ Outer ] 区切りに大きい三角とギザギザを追加 【確認中】[ Outer ] 区切りに大きい三角とギザギザを追加 Jul 12, 2024
@drill-lancer drill-lancer changed the title 【確認中】[ Outer ] 区切りに大きい三角とギザギザを追加 【2人目確認中】[ Outer ] 区切りに大きい三角とギザギザを追加 Jul 12, 2024
@drill-lancer
Copy link
Member

@mtdkei
リンクの area-label の部分が 翻訳の影響でリカバリが発生するのが気になりましたが、
翻訳の影響なので個人的に仕方がないとのことで承認します。
ありがとうございました。

@goutetsuguma
Copy link
Contributor

goutetsuguma commented Jul 12, 2024

調整ありがとうございます!ギザギザも大きい三角もよく使う区切りなので良いですね。

2人目確認中ですが、使ってみて気になったところをお伝えします。
上部と下部の区切りレベルを大きくするとpadding-top、padding-bottom がつきますが、ギザギザの時だけは区切りのギザギザ部分のpathの高さがかわらないため、レベルを大きくすると余白が目立つのが気になりました。他の区切りタイプも同様にpaddingがつくので、仕様かなとも思いました。

2人目の方確認お願いします。

@mtdkei
Copy link
Contributor Author

mtdkei commented Jul 12, 2024

@goutetsuguma
ありがとうございます。確かに気になったのでギザギザを調整してみました。
お手数ですが今一度ご確認いただけた幸いです。

@mthaichi
Copy link
Contributor

@mtdkei ありがとうございます。これはいいですね!

deprecated不要だと思います。
deprecatedは「設定値(attributesの値)が同一であることが前提で、saveで出力されるHTMLが変更される場合」に指定してください。「壊れるかもしれないので念の為deprecated」はコードの肥大化につながるので、できればやめたいところです。
attributesが増える。選択できる設定値が増えるだけなら、deprecatedは不要で、このルールでやっていってリカバリが発生するならそれは仕方がないと考えて良いと思います。
もし、壊れるケースがあれば、具体的にそのケースを示してください。

あと、翻訳は原則別ブランチにして頂いたほうがよいかなと。
ここでは、#2101 の反映がされていないかもしれないので、最新のdevelopをマージして、改めて翻訳ファイルを作成し、他のプルリクに翻訳がなければ、このブランチに混ぜてマージしてもいいと思います。

リンクの area-label の部分が 翻訳の影響でリカバリが発生するのが気になりましたが、

@drill-lancer 理論上は言語を切り替えたときにリカバリが発生することになりますので、本ブランチとは関係ないことかなと思います。aria-labelの値そのものを別の機会に見直したほうがいいかもしれませんね。

@goutetsuguma
Copy link
Contributor

@mtdkei ありがとうございます!タイプがギザギザの時に区切りレベルを変更してもpaddingが10px(ギザギザの高さ)と固定されていることを確認しました!

@mtdkei
Copy link
Contributor Author

mtdkei commented Jul 12, 2024

@mthaichi
ありがとうございます。ここで追加したdeprecatedを削除しました。
明確にしてくださりありがとうございます。今後のdeprecatedや翻訳はそのように運用いたします。

@mthaichi
Copy link
Contributor

@mtdkei 修正ありがとうございます!
テスト(test/ 以下)も無しで良いですね。ここのテストはそれこそリカバリが発生しないかどうかのテストなので、deprecated対象外ならいらないかなと思います。

と書きましたが、 SVG使ってる複雑なパターンですので、別系統でテストがあってもいいですね。
vk-blocks__outer__border.html を作成し、区切り付きのブロックパターンのHTMLをいれて、npm run test-unit でテストが通ればOKです。headingなどもid_classといった別系統のテストがありますので、ご参考まで)
今後deprecatedが発生する度に全系統のテストを更新する必要はあります。

テストいれるかどうかはおまかせします。もしやってみたいということであればチャレンジしてみてください。

@mtdkei
Copy link
Contributor Author

mtdkei commented Jul 12, 2024

@mthaichi
ご確認ありがとうございます。vk-blocks__outer__deprecated-1-78-0.html を削除しました。
また、今後も区切り線を増やしていく予定なので vk-blocks__outer__divider.html を追加しました。今は大きい三角とギザギザ区切り線にしたOuter二つを入れています。
今後、新しい区切りができたら順次テストにも追加する予定で考えてますが、このような運用で何か問題がありそうでしたらお知らせください。

@mthaichi
Copy link
Contributor

@mtdkei 確認遅くなってすみません。OKだと思います!
readmeと翻訳がコンフリクトしておりますので、最新のdevelopをこのブランチにマージして、コンフリクトを解消した後、プッシュして問題なければ、developにマージして良いと思います。(コンフリクトがでたのは、私の確認が遅くなったせいかもしてません。すみません)

@mtdkei
Copy link
Contributor Author

mtdkei commented Jul 16, 2024

@mthaichi
ご確認ありがとうございます。こちらこそお手数をおかけいたしましたが、大変勉強になりました。
それではご教示いただいた方法でマージをしてみます。

@mtdkei mtdkei merged commit cf578c1 into develop Jul 16, 2024
14 checks passed
@mtdkei mtdkei deleted the add/outer/border/serrated branch July 16, 2024 08:09
@kurudrive kurudrive changed the title 【2人目確認中】[ Outer ] 区切りに大きい三角とギザギザを追加 [ Outer ] 区切りに大きい三角とギザギザを追加 Jul 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.

4 participants