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のリンク設定が配信されるまで一旦draft/確認中】[ Outer ] 背景画像に焦点ピッカー追加 #2255

Draft
wants to merge 56 commits into
base: develop
Choose a base branch
from

Conversation

mtdkei
Copy link
Contributor

@mtdkei mtdkei commented Oct 4, 2024

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

#2102
#1012

どういう変更をしたか?

PC、タブレット、モバイルの背景画像に焦点ピッカーを追加しました。

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

変更前 Before

スクリーンショット 2024-10-10 15 19 07

変更後 After

image

実装者の確認事項

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

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

プログラムの変更の場合

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

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

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

1. 背景画像設定時のフォーカルポイント自動ON
PC、タブレット、モバイル:各デバイスで背景画像を設定すると、自動的にEnable Focal PointがONになることを確認。各デバイスのフォーカルポイント設定が独立して管理されていることも確認。

2. 背景画像削除時のフォーカルポイントのOFF
PC、タブレット、モバイル:背景画像を削除すると、それぞれのEnable Focal Pointが自動的にOFFになることを確認。他のデバイスに影響がないことも確認。

3. 背景画像の設定・削除の繰り返し時の挙動
PC、タブレット、モバイル:画像設定と削除を繰り返しても、Enable Focal Pointが正しくOFF → ONに切り替わることを確認。また、設定が他のデバイスに影響しないことを確認。

4. トグルの無効化状態の確認
PC、タブレット、モバイルで背景画像がない時は、それぞれのEnable Focal Pointトグルが無効化され、画像を設定すると有効化されることを確認。

単体の背景画像設定に関する確認事項

1. PC用背景画像のみが設定されている場合

  • PC用背景画像のみが設定されている場合でも、モバイルやタブレットのEnable Focal Pointトグルが操作可能であることを確認し、各デバイスごとに異なるフォーカルポイントの設定が可能であることを確認。
  • モバイルとタブレットのフォーカルポイントがOFFの時、PCのフォーカルポイントを継承することを確認。

2. タブレット用のみ背景画像が設定されている場合

  • PCでは非活性化し、モバイルでもEnable Focal Point MobileがONにできることを確認し、各デバイスごとに異なるフォーカルポイントの設定が可能であることを確認。
  • PCとモバイルのフォーカルポイントタブレットのフォーカルポイントを継承することを確認。

3. モバイル用のみ背景画像が設定されている場合

  • PCとタブレットのEnable Focal Pointトグルは非活性化になることを確認。
  • PCやタブレットのフォーカルポイントがモバイルのフォーカルポイントを継承する挙動を確認。

デバイスごとの組み合わせによる背景画像設定に関する確認事項

1. PCとタブレットとモバイル

  • PCとタブレットとモバイルに背景画像が設定された場合、Enable Focal Point PCとEnable Focal Point TabletとEnable Focal Point Mobileが自動的にONになることを確認。
  • PC、タブレット、モバイルごとに異なるフォーカルポイントの設定が可能であることを確認。
  • いずれかのEnable Focal PointをOFFにした場合、それぞれの上位設定のフォーカルが当たることを確認。(PCの場合はデフォルトの50%、50%のまま)
  • 設定を保存した後も、各デバイスごとのフォーカルポイント設定が保持されることを確認。

2. PCとタブレット

  • PCとタブレットの両方に背景画像が設定された場合、Enable Focal Point PCとEnable Focal Point Tabletが自動的にONになることを確認。
  • PC、タブレットごとに異なるフォーカルポイントの設定が可能であることを確認。
  • モバイル用画像がない場合でも、Enable Focal Point Mobileトグルが操作可能であり、タブレットまたはPCのフォーカルポイントを継承することを確認。
  • 設定を保存した後も、各デバイスごとのフォーカルポイント設定が保持されることを確認。

3. PCとモバイル

  • PCとモバイルの両方に背景画像が設定された場合、Enable Focal Point PCとEnable Focal Point Mobileが自動的にONになることを確認。
  • PC、モバイルごとに異なるフォーカルポイントの設定が可能であることを確認。
  • タブレット用画像がない場合でも、Enable Focal Point Tabletトグルが操作可能であり、PCのフォーカルポイントを継承することを確認。
  • 設定を保存した後も、PCとモバイルのフォーカルポイント設定が正しく保持されることを確認。

4. タブレットとモバイル

  • タブレットとモバイルの両方に背景画像が設定された場合、Enable Focal Point TabletとEnable Focal Point Mobileが自動的にONになることを確認。
  • タブレット、モバイルごとに異なるフォーカルポイントの設定が可能であることを確認。
  • モバイル用画像がない場合、Enable Focal Point Mobileトグルが操作可能であり、タブレットのフォーカルポイントを継承することを確認。
  • 設定を保存し、再読み込みしてもタブレットとモバイルのフォーカルポイント設定が保持されることを確認。

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

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

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

実装者と同じ確認紙をしてください。今回は背景画像との連携で確認項目が多くなっているため、以下のようにしていただくとご負担が減るかと思われます。

  • 1人目: 挙動全般の確認
  • 2人目: コード全般の確認

レビュワー向け

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

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

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

@mtdkei mtdkei changed the title Add/outer/focal point picker [ Outer ] 背景画像に焦点ピッカー追加 Oct 7, 2024
@mtdkei
Copy link
Contributor Author

mtdkei commented Oct 24, 2024

@akito-38
ありがとうございます。そちらの設定が抜けてました。
ただいま修正しましたのでご確認いただけたらと思います。

また、タブレット時の背景画像の仕様についてですが、この辺りは @kurudrive さんの方でご確認いただけたらと思いますが、おそらくこの仕様なのではないかと思います。
VK製品はユーザー時代より長年使っていますが、タブレットのみの設定をしている場合でもPCやmobileの画面幅の時でも背景画像は設定されていました。

@akito-38
Copy link
Contributor

@mtdkei
image
すみません。
もう一つありました。
この設定の時、確認事項に
「PC用画像がない場合、PCのEnable Focal Pointトグルが操作可能であり、タブレットのフォーカルポイントを継承することを確認。」
とありますが、PCのEnable Focal Pointトグルが操作できませんでした。
確認いただけるでしょうか?
認識まちがってたらすみません。

@mtdkei
Copy link
Contributor Author

mtdkei commented Oct 24, 2024

@akito-38
完全にこちらの説明ミスです。お手数をおかけして申し訳ありません。
正しくは以下になります。

  • モバイル用画像がない場合、Enable Focal Point Mobileトグルが操作可能であり、タブレットのフォーカルポイントを継承することを確認。

プルリクの方の説明も直しました。ありがとうございます。

Copy link
Contributor

@akito-38 akito-38 left a comment

Choose a reason for hiding this comment

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

挙動全般の確認を確認しました。修正ありがとうございます。

@akito-38
Copy link
Contributor

@MasayaMORIMOTO @mtdkei
私は挙動全般を確認しましたが、森本さんはどちらでしょうか?
動作確認という事は挙動でしょうか?
だとしたらもう一方、コード全般の確認もしないとですね。

@mtdkei
Copy link
Contributor Author

mtdkei commented Oct 25, 2024

@akito-38 @MasayaMORIMOTO
(CC: @kurudrive @drill-lancer @mthaichi)
動作確認を行なっていただきありがとうございました。引き続きコード全般の確認も行っていただけるなら幸いです。
念の為、他の開発の方にもメンションしていますので、どなたか可能な方がコード全般の確認も行なっていただければと思います。

@mthaichi
Copy link
Contributor

mthaichi commented Oct 30, 2024

@mtdkei 修正お疲れ様です!
宮本さんと一緒にちょっと見て気になった点です。

  • コンフリクト解消してください🙏
  • 今回、後方互換(deprecated)いりますか?

以上、ご確認をお願いします。

@mtdkei
Copy link
Contributor Author

mtdkei commented Oct 30, 2024

区切りがマージされたからコンクリフトがおきたのですね、あとで行います。
今回、src/blocks/_pro/outer/GenerateBgImage.jsやsave.jsを変更したのでいると思ったのですが、何か認識が異なることがあればおっしゃっていただけたら幸いです。

@mthaichi
Copy link
Contributor

区切りがマージされたからコンクリフトがおきたのですね、あとで行います。 今回、src/blocks/_pro/outer/GenerateBgImage.jsやsave.jsを変更したのでいると思ったのですが、何か認識が異なることがあればおっしゃっていただけたら幸いです。

@mtdkei なるほど、必要ですね。
では、コンフリクトだけお願いします。

@mthaichi mthaichi self-requested a review October 30, 2024 09:19
*/

// 1.76.0 から attributes を変更
const blockAttributes9 = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@mtdkei ここは古いattributesが定義されなければならないはずですが、bgFocalPointPCなどは今回の修正で加わったんですよね? 単純に blockAttributes8 を参照すればよいと思いますが、いかがでしょう。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ありがとうございます。const blockAttributes9 はコメントアウトいたしました。

@mtdkei
Copy link
Contributor Author

mtdkei commented Nov 5, 2024

Outerの区切り線がリリースされるまで一旦Draftにします。

@mtdkei mtdkei marked this pull request as draft November 5, 2024 01:40
@mtdkei mtdkei marked this pull request as ready for review November 19, 2024 23:46
@mtdkei
Copy link
Contributor Author

mtdkei commented Nov 19, 2024

@mthaichi
コンクリフト解消しました。お手数ですが、こちらでOKそうでしたらマージいただけたらと思います。

@mtdkei mtdkei changed the title 【確認中】[ Outer ] 背景画像に焦点ピッカー追加 【outerのリンク設定が配信されるまで一旦draft/確認中】[ Outer ] 背景画像に焦点ピッカー追加 Dec 12, 2024
@mtdkei mtdkei marked this pull request as draft December 12, 2024 01:25
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