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]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 #1793

Merged
merged 19 commits into from
Sep 21, 2023

Conversation

osmdik
Copy link
Contributor

@osmdik osmdik commented Aug 22, 2023

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

#1623

どういう変更をしたか?

  • Outer区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加しました。

実装者の確認事項

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

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

プログラムの変更の場合

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

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

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

実装者が確認した手順を箇条書きで記載してください。

  • Outerブロックでこれまで通り、端末共通で区切りレベルを変えられることと正常に反映されることを確認
  • 共通で設定をオフにして、端末毎に設定できること、それぞれの端末で正常に反映されることを確認

確認URL

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

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

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

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

レビュワーがどういう手順で何を確認して欲しいかを記載してください。

  • Outerブロックでこれまで通り、端末共通で区切りレベルを変えられることと正常に反映されることを確認
  • 共通で設定をオフにして、端末毎に設定できること、それぞれの端末で正常に反映されることを確認

レビュワー向け

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

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

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

@osmdik osmdik changed the title 【WIP】[Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 [Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 Aug 25, 2023
@osmdik osmdik changed the title [Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 【確認待ち】[Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 Sep 6, 2023
@osmdik osmdik marked this pull request as ready for review September 6, 2023 05:21
@sysbird sysbird changed the title 【確認待ち】[Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 【確認中】[Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 Sep 8, 2023
@sysbird
Copy link
Member

sysbird commented Sep 8, 2023

@osmdik
確認中です、

  • [端末毎に共通で設定] OFFで、デバイスごとの設定が正しく表示されます(1)
  • [端末毎に共通で設定] ONで、正しく表示されます(2)
  • 過去に作成した Outer は(2)で表示されると思うので、こちらも表示されます

1点気になります

  • パターンライブラリからコピペしたブロックは(2) のはずですが、(1)としてペーストされてしまい、区切りの設定が初期化されてしまいます (→例えば傾斜していたものは、傾斜なしになる)
  • [端末毎に共通で設定] はデフォルトON なのでうまくいきそうな気もしますが、いまのところ原因はわかりません
  • コピペしてるパターンは ヒーローエリア_フィットネス_採用_カスタムCSSや、トップページ_キャンプサイト_観光・旅行 などです
  • ブラウザのキャッシュはクリアしてます、私ももう少し触ってみます

【追記】
パターンをコピペしいったん保存、リロードした際に、ブロックのリカバリーが表示されます
リカバリーすると解消されて、もとの区切り設定が有効になります。
deprecated の問題でしょうかね?

@sysbird sysbird changed the title 【確認中】[Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 【返信待ち】[Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 Sep 11, 2023
@osmdik osmdik changed the title 【返信待ち】[Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 【確認待ち】[Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 Sep 13, 2023
@osmdik
Copy link
Contributor Author

osmdik commented Sep 13, 2023

@sysbird
確認ありがとうございます。
互換処理を修正したので、再度確認をお願いいたします。

@sysbird
Copy link
Member

sysbird commented Sep 14, 2023

@osmdik
区切りの設定を端末ごとに指定できるのを確認しました
過去のブロックや、パターンライブラリからコピペしたブロックでも問題なく表示されたり、編集できます。
対応ありがとうございました

2人め確認をおねがいします

@sysbird
Copy link
Member

sysbird commented Sep 14, 2023

@osmdik @kurudrive @goutetsuguma

ちょっと気になるところ
→ 私だけかもしれませんので、すでに石川さんと詰められた仕様でしたらスルーでOKです〜

  1. 「端末毎に共通で設定」
    「端末毎」なのに「共通」?どっちだろう?と、毎回ここで立ち止まってしまいます。
    下記の例はいかがでしょうか?
     (設定値も逆になってきたりと面倒かもしれませんね)
  • 端末毎に設定
  • すべての端末で共通
  1. 端末ごとの設定値の反映
    「端末毎」に設定の表示は変化しているので動作は正しいですが、使いかたがあまりわかりません。
    とくに三角は、モバイルのとき少なめにすると三角が小さすぎたり、そうかといって大きめにすると高さが出てしまったり、ちょうどよい使い方がわかりませんでした
    issue を挙げた @goutetsuguma さんに見てもらえると助かります

@sysbird sysbird changed the title 【確認待ち】[Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 【2確認待ちとご意見待ち】[Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 Sep 14, 2023
@osmdik
Copy link
Contributor Author

osmdik commented Sep 14, 2023

「端末毎に共通で設定」
「端末毎」なのに「共通」?どっちだろう?と、毎回ここで立ち止まってしまいます。
下記の例はいかがでしょうか?
 (設定値も逆になってきたりと面倒かもしれませんね)
端末毎に設定
すべての端末で共通

確かにわかりにくいなと思ったので、「端末毎に設定」に変更し、設定値も調整しました。

端末ごとの設定値の反映

端末毎に設定した値の反映状況については、編集画面の端末毎のプレビューで確認してもらうのが最適だと思いますがいかがでしょうか?

@kurudrive @goutetsuguma
想定・希望している仕様的にご意見などよろしくお願いいたします。

@goutetsuguma
Copy link
Contributor

goutetsuguma commented Sep 19, 2023

調整ありがとうございます!めちゃくちゃ使いやすいです!さすがです!!

少しだけ気になったところを書きます。
区切りレベルに0以外の何か数値が設定してある状態【※01】で「端末毎に設定」をONにすると、PC・タブレット・モバイル の数値が「0」になりますが【※02】、すでに設定されている数値がPC・タブレット・モバイルの全てに入ると 調整しやすいかもしれないなと思いました。(既存のパターンの場合は、モバイルとタブレットの数値を調整したい場合が多いので、PCのところに再度数字を入れなくてはいけないので、その手間が省ける)

【※01】区切りレベル-100 が入っている
Outer-kugiri01

【※02】「端末毎に設定」をONにするとPC・タブレット・モバイルに0が入る
Outer-kugiri02

↑この例の場合だと、「端末毎に設定」をONにするとPC・タブレット・モバイルに-100 が入る。

でも実装が難しい場合や、0にリセットされる方がいいという意見もあると思いますので、他の方のご意見も聞いてみたいとおもいます🙇‍♀️

@kurudrive いちど石川さんもご確認をお願いしたいです🙇‍♀️
よろしくお願いします。

@kurudrive
Copy link
Member

@goutetsuguma @osmdik 確認しました

すでに設定されている数値がPC・タブレット・モバイルの全てに入ると 調整しやすいかもしれない

そうですね、0にリセットされると元の数値がいくつかわからなくなってしまうので、久納さん案が良いと思います。

よろしくお願いいたします。

@osmdik
Copy link
Contributor Author

osmdik commented Sep 19, 2023

@goutetsuguma @kurudrive
調整しました。確認をお願いいたします。

@goutetsuguma
Copy link
Contributor

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

@kurudrive @osmdik
私の環境ですと、修正していただいたものをpullしてbuildして、ページを再読み込みして確認してみたのですが、「端末毎に設定」をONにすると0にリセットされてしまうようでした。私のローカル環境の問題かもしれませんので(npm installでエラーがでるのが直っていない、、)、お忙しいと思いますが 石川さん 確認をお願いしても良いでしょうか🙇‍♀️

@kurudrive
Copy link
Member

@osmdik 0になってしまう空気を感じます(・w・;
これは大嶋さんの環境では 0 にならない状態です?

2023-09-19.11.43.01.mov

@osmdik
Copy link
Contributor Author

osmdik commented Sep 19, 2023

パターンで試していたため、初期値が0の場合を考慮していませんでした。
端末毎の設定値すべてが0の場合にも共通の設定値が引き継がれるよう調整しました。
お手数おかけしますが、再度確認をお願いいたします。

@kurudrive
Copy link
Member

kurudrive commented Sep 19, 2023

@osmdik 僕の端末では下記になりますが、大嶋さんの環境ではこうならない状態でしょうか?

  1. 区切りのレベルを50に指定
  2. 端末毎に設定 を有効
  3. 各端末に 50 が反映されるのではなく 0 になってしまう
2023-09-19.15.44.00.mov

@osmdik
Copy link
Contributor Author

osmdik commented Sep 19, 2023

@kurudrive @goutetsuguma
修正しました。確認をお願いいたします。

@goutetsuguma
Copy link
Contributor

goutetsuguma commented Sep 20, 2023

@osmdik @kurudrive

いろいろありがとうございます🙇‍♀️私のローカル環境で確認したのですが、「端末毎に設定」をONにすると、うまく区切りレベルの数字が引き継がれるものと、引き継がれないものがありました。

【数字が正常に入るもの】
原因がまだよくわからないのですが、過去に作ったパターンだとうまく数字が入りました。

【数字が正常に入らないもの】

  • 新規でOuterブロックを配置した場合
    この場合はうまく数字が入らないようでした。上部区切りレベルを100にしてから「端末毎に設定」をONにしたところ、上部区切りレベルの「PC・タブレット・モバイル」の数値に「100」ではなくて「4」とはいりました。

  • 非公開: アプリ商品紹介
    まだ非公開のこのパターンだと、「端末毎に設定」をONにすると0にリセットされてしまうようでした。


最近設置したOuterだと、うまく数字が入らないのかな、、???
でも、わたしのローカルが原因の可能性もありますので、石川さんに確認をお願いしてもいいでしょうか?
お忙しいところすみません>< よろしくおねがいします

@kurudrive
Copy link
Member

@osmdik @goutetsuguma

僕も 0 ではなくなったけど入力した値とは違う数字が入るようになりました。

2023-09-20.15.08.43.mov

@osmdik
Copy link
Contributor Author

osmdik commented Sep 21, 2023

失礼しました。
端末毎の設定値が同じ場合に共通設定の値が変更されたとき、共通設定の値を引き継ぐ仕様に変更しています。
改めて確認をお願いいたします。

@goutetsuguma
Copy link
Contributor

@osmdik @kurudrive

修正ありがとうございます!!新規のも既存のパターンも問題なく数字が引き継がれることを確認しました!

石川さんも念の為、ご確認と問題なければMergeをお願いいたします🙇‍♀️

(余談:非公開のアプリのパターンで数字が引き継がれないのですが、このパターンだけがおかしいので、わたしの作りに問題があるようでした。)

@kurudrive
Copy link
Member

@osmdik 数字が引き継がれる事を確認しました。ありがとうございます。

@kurudrive kurudrive merged commit 92088c7 into develop Sep 21, 2023
17 checks passed
@kurudrive kurudrive deleted the add/outer/diveider-device-option branch September 21, 2023 05:19
@osmdik osmdik changed the title 【2確認待ちとご意見待ち】[Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 [Outer]区切りの設定を端末毎(mobile/tablet/pc)に設定できるようにオプション追加 Sep 21, 2023
@shimotmk shimotmk mentioned this pull request Sep 21, 2023
6 tasks
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