-
Notifications
You must be signed in to change notification settings - Fork 2
コアブロックのナビゲーションメニューの縦並び時の不具合修正 #245
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
Conversation
サブメニューの表示状態を変更
サブメニューを含むメニュー項目の下矢印を横並びにする(テスト含む)。
メニュー項目のテキストと下矢印を横並びにするためにnotを付与
サブメニュー項目のaのpaddingを追加
is-vertical の padding が 0 になる影響で submenuのa が効かない現象に対応
gap: 0; があることで、ブロックエディター上のナビゲーションメニューに設定している gap が効かない現象に対応
コアブロックのナビゲーションメニューのreadme記述
編集画面でサブメニューの背景色を設定していない時、背景色が透明になってしまう現象に対応
ブロックエディター上でサブメニューの色を設定していない時、背景が透明になる現象に対応
@mtdkei @kurudrive |
assets/_scss/_navigation.scss
Outdated
// --wp--style--block-gap: 0; | ||
// gap: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
とりあえず gap が効かない問題はこのブランチから切り離して、別の issue / プルリク とした方が良いかなと思います(・w・
assets/_scss/_navigation.scss
Outdated
& .wp-block-navigation-submenu a:not(.wp-element-button), | ||
& .wp-block-navigation-submenu button.wp-block-navigation-item__content, | ||
& .wp-block-pages-list__item button.wp-block-navigation-item__content { | ||
padding: .5em 1em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtdkei これって、他のセレクタからの padding:0.8em 指定に負けて効いてなくないです(・・?
@kurudrive gapが効かない部分はどうしましょう。確かに既存の縦並びメニューにも影響がありそうで難しいです。 |
パターンの修正を確認しており、こちらのプルリクを確認しました。 gapが効かなかった件を修正しました。一度さらっと見ていただきたいと思いますので、 石川さんが確認途中でしたので石川さん確認待ちとします🙇 |
@mtdkei ありがとうございます。 |
@kurudrive |
assets/_scss/_navigation.scss
Outdated
@@ -75,7 +75,7 @@ | |||
border: none; // コアが border を付与してくるので打ち消す | |||
} | |||
|
|||
.wp-block-navigation__responsive-container{ | |||
.wp-block-navigation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtdkei / Cc: @goutetsuguma @sysbird
変更ありがとうございます!
対象セレクタのクラス名を変更する場合、
もともとなぜそのセレクタになっていたのかを慎重に考えてみましょう(・w・b
この場合、"__responsive-container" が対応になってますが...
よくよく考えたらこのCSSは常時表示のメニュー(ハンバーガーメニューに切り替わらないメニュー)でも当てないといけない内容でしたので、それに気づいてなくて __responsive-container をセレクタにしていた僕の認識間違いです。
なので、このクラス名の変更自体は正しいです。ありがとうございます。
ただ、ここを変更するという事は 他に __responsive-container をセレクタにしている箇所がある場合は同じ勘違い・不具合が発生している可能性が考えられます。
となってくると、これは "縦並びの不具合修正" とは別の意図になってきますので、別のプルリクにしました。
#314
実際こういった感じで、テーマのCSSの修正は元の意図がわかりにくかったり、得にこのテーマはフルサイト編集が初期の頃に手探りで作ったものなので、現行の仕様では不要になった指定や、単純な僕の間違いなども含んでいる事もあり、何気に修正が難しいです。
各意図を把握した上でプルリクを分けないと、後から「なんでこれに指定したんだろう」と思った時も追いにくくなるので、注意しましょう。
実際こういう修正作業は難しいというか、「他人が作ったモノの意図なんかわかるかい!」という部分も多分に含んでしまうので、ものすごく触りにくいと思います。
「そんなん作った石川さんじゃないと気づかないじゃん」案件だと僕も思います。
それを踏まえて、他の人が見て意図がわかるようにテーマのCSSでは特にコメントなど工夫したり、プルリクを細かく区切って出した方がいいかもしれません。
無理難題な気は僕もしているのですが、テーマのCSSを変更する時はそのあたりも気にしつつよろしくお願いいたします。
( ̄人 ̄)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確かにおっしゃる通りです。ありがとうございます。
プルリクとしてこちらに出していいものか迷ってた部分がありましたのでご確認いただけて幸いです。
今後クラス名を変更する場合はこの点を特に気をつけてみてみたりコメントアウトを残します。
# Conflicts: # readme.txt
#315 の後で再確認 |
# Conflicts: # readme.txt
@kurudrive ナビゲーションリファクタリングのプルリクがCloseになっていますので、このプルリクはもう閉じても大丈夫でしょうかね? |
@mtdkei はい、せっかく調整いただいたのにすみません ( ̄人 ̄) |
チケットへのリンク / 変更の理由(元のissueがあればリンクを貼り付ければOK)
#244
どういう変更をしたか?
以下の点を修正しました。
実装者はレビュワーに回す前に以下の事を確認してチェックをつけてください。
ソースコードについて
プログラムの変更の場合
テストを書かないのは普通ではありません。書けるテストは極力書くようにしてください。
書いていない場合は書かない理由を記載してください。
その他
変更内容について何を確認したか、どういう方法で確認をしたかなど
レビュワーの確認方法・確認する内容など
フッターのパターンブロックをコピペして以下をご確認ください。
レビュワーに回す前の確認事項
レビュワー向け
確認して変更が反映されていない場合の確認事項