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

[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 #2166

Merged
merged 48 commits into from
Oct 21, 2024

Conversation

mtdkei
Copy link
Contributor

@mtdkei mtdkei commented Aug 9, 2024

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

#2136

どういう変更をしたか?

ブロック挿入時に初期状態をdeleteからdisplayに変更しました。

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

変更前 Before

image

変更後 After

image

実装者の確認事項

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

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

プログラムの変更の場合

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

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

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

  • 編集画面でグリッドカラムカードを設置したときにカラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトが「表示」になっていることを確認しました。
  • 既存のグリッドカラムカードに影響がないことを確認しました。
  • WP6.5、6.6で確認しました。

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

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

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

実装者と同じ実装を行なってください。また、開発の方はコードの確認もお願いいたします。
今回はdeleteからdisplayに変えた方が使いやすいかどうかのご確認も含めているため、普段レビュワーではない方も含めています。実際に実装するかどうかも含めてご検討ください。
よろしくお願いいたします。


レビュワー向け

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

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

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

@sysbird sysbird changed the title 【確認&ご意見待ち】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 【確認中&ご意見待ち】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 Aug 13, 2024
@sysbird
Copy link
Member

sysbird commented Aug 13, 2024

@mtdkei
確認中です
デフォルトで表示はわかりやすくてよいと思います
最初から隠れているとユーザーは気づきにくいですもんね

気になるところ
以前に作成したグリッドカラムカードをこのブランチで編集しようとすると、[削除]だったところが、[表示]になってしまうようです。たぶん以前は値がないとき「削除」だったのが「表示」に変更したためでしょうかね
あれ、これどうなったらいいんでしょう…難しいです

@sysbird sysbird changed the title 【確認中&ご意見待ち】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 【確認中・回答待ち&ご意見待ち】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 Aug 13, 2024
@mtdkei
Copy link
Contributor Author

mtdkei commented Aug 14, 2024

@sysbird
おっしゃる通り、既存のブロックにも影響があるようでした。
色々調整しているのですが、既存のブロックでdeleteにしている時でもdisplayになってしまう部分の対応がうまくいきません。

もしデフォルトを「表示」にする方向で進めていい場合は開発の方にもこの辺を見ていただけたら嬉しいのですが @kurudrive さんはどう思いますか?

@mtdkei mtdkei changed the title 【確認中・回答待ち&ご意見待ち】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 【確認中・ご意見待ち】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 Aug 14, 2024
@sysbird
Copy link
Member

sysbird commented Aug 15, 2024

@mtdkei
案でなくて 現状報告でおそれいいります〜

既存のブロックで delete はデフォルトなので、headerDisplay がないときは delete 扱い
このブランチでは display がデフォルトなので、headerDisplay がないときは display 扱い
デフォルト値は保存されてないためと思います

先日、
[ グリッドカラムカード ] 編集モードのデフォルト値を「すべてのカラム」に #1992
の仕様変更がありましたが、この場合は編集モードは保存されていなくて、初期表示をどうするか?だった気がします

@mtdkei
Copy link
Contributor Author

mtdkei commented Aug 15, 2024

@sysbird
ご指摘の通り、既存のブロックがdeleteの設定のまま新しいデフォルト値displayに影響されてしまう点について、こちらも対応がうまくいっておりません。この問題を解決するために、デフォルトを「表示」に変更する方向で進める場合、皆さんのご意見をいただけると助かります。@kurudrive さんは、この問題についてどのようにお考えでしょうか?

@kurudrive
Copy link
Member

kurudrive commented Aug 23, 2024

@mtdkei @sysbird 表示で良いと思います(・w・b

@kurudrive kurudrive changed the title 【確認中・ご意見待ち】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 【調整待ち?確認待ち?】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 Aug 26, 2024
@mtdkei mtdkei marked this pull request as draft August 26, 2024 07:45
@mtdkei
Copy link
Contributor Author

mtdkei commented Oct 3, 2024

@mthaichi @sysbird
皆さんのご意見を参考に、data属性からclassNameにしてみました。
こちらの方が設定としては自然かな?と思いますがいかがでしょうか?
なお、deleteの時のクラス名がすでに設定されているようだったので以下のように変えております。

className: classnames(containerClasses, {
	'vk_gridcolcard_item-noHeader': headerDisplay === 'delete',
	'vk_gridcolcard_item-noFooter': footerDisplay === 'delete',
	[`vk_gridcolcard_item-header-${headerDisplay}`]:
		headerDisplay !== 'delete',
	[`vk_gridcolcard_item-footer-${footerDisplay}`]:
		footerDisplay !== 'delete',
}),

@mtdkei mtdkei changed the title 【仕様検討中 / 確認中】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 【確認中】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 Oct 3, 2024
@sysbird
Copy link
Member

sysbird commented Oct 15, 2024

@mtdkei
再度確認しました
デフォルトが変更になり、問題なく動作していると思います
ご検討ありがとうございます

細かいことで恐縮ですが気になる点
■ グリッドカラムカードアイテムフッター内のボタンの配置がデフォルトで異なる気がします
[dev] ボタン 左寄せ
[当ブランチ] ボタン 中央寄せ → ブロックを中央寄せに設定しているスタイルがあるので、仕様変更でしょうか?

■ deprecated の番号が混在しているかもしれない?
・/gridcolcard-item/deprecated/1.84.0 と /gridcolcard-item/deprecated/1.85.1 と2つ増えている
・/gridcolcard-item-footer/deprecated/1.83.0/
・今の段階ではひとつ前のバージョンは 1.85.1 になるかと

@mtdkei
Copy link
Contributor Author

mtdkei commented Oct 15, 2024

@sysbird
ありがとうございます!すみません、ボタンの件は試してみたものが混在してました。こちらは削除します。
また、deprecatedも調整してみます。

@mtdkei
Copy link
Contributor Author

mtdkei commented Oct 15, 2024

@sysbird
修正しました
いつもありがとうございます!

@sysbird
Copy link
Member

sysbird commented Oct 17, 2024

@mtdkei
対応ありがとうございます!
下記について確認しました

  • ヘッダーメディアエリアとカラムフッターボタンエリアのデフォルトが「表示」になっている
  • 過去に設置したブロックに影響ない
  • ボタンの位置がデフォルトで従来どおり
  • deprecated の番号

@mthaichi
見ていただけますでしょうか〜

@sysbird sysbird changed the title 【確認中】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 【2人め確認待ち】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 Oct 17, 2024
@goutetsuguma goutetsuguma changed the title 【2人め確認待ち】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 【丸山さん確認待ち】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 Oct 18, 2024
@mthaichi mthaichi merged commit 5c912f7 into develop Oct 21, 2024
14 checks passed
@mthaichi mthaichi deleted the fix/gridcolcard-header-footer-display branch October 21, 2024 09:57
@mthaichi
Copy link
Contributor

@mtdkei ありがとうございます。改めて動作確認させていただき、問題ないと思いますのでマージします。
お待たせしてすみませんでした。(・人・)

@mthaichi mthaichi changed the title 【丸山さん確認待ち】[ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 [ グリッドカラムカード ] カラムヘッダーメディアエリアやカラムフッターボタンエリアのデフォルトを「表示」に変更 Oct 21, 2024
@mtdkei
Copy link
Contributor Author

mtdkei commented Oct 21, 2024

@mthaichi
ご確認いただきありがとうございました!

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