[ アイコン ]ツールバーからのリンク指定機能にnoreferrer、nofollow、link descriptionオプションを追加

[akito-38]

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

#2199

どういう変更をしたか？

リンクツールバーに Edit link を追加し、a タグの rel やアクセシビリティ対応のテキストを設定できるようにしました。

・Add noreferrerと Add nofollow: rel に noreferrer と nofollow をそれぞれ設定できます。外すことも可能です。
・Accesibility link description: アクセシビリティ対応のテキスト設定にテキストを入れると、spanにテキストが入ります。デフォルトでは 「ブロック名 ＋ link」のテキストが設定されます。

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

変更後 After

実装者の確認事項

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

• 複数の意図の変更 （ 機能の不具合修正 + 別の機能追加など ） を含んでいないか？
• Files changed (変更ファイル)の内容は目視で確認したか？
• readme.txt に変更内容は書いたか？
• readme.txt に記載の変更内容はエンドユーザーが見て変更の概要がわかるように書かれているか？
• 本当にちゃんと確認をしたか？

プログラムの変更の場合

• 書けそうなテストは書いたか？　省略

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

aria-label属性が削除されていることを確認しました。
「リンクを別ウィンドウで開く」のチェックがない場合、targetの出力がないことを確認しました。
「リンクを別ウィンドウで開く」のチェックがある場合、targetが出力され、「_blank」が入ることを確認しました。
「noreferrer を追加」「nofollow を追加」のチェックがない場合、relの出力がないことを確認しました。
「noreferrer を追加」「nofollow を追加」のチェックがある場合、relが出力され、「noreferrer」「nofollow」がそれぞれ入ることを確認しました。
編集画面で「リンクの説明」が空欄の場合、「screen-reader-text」クラスを持つspanタグの中に、「ブロック名 ＋ link」のテキストが出力されている事を確認しました。
編集画面で「リンクの説明」に適当なテキストを入力し「screen-reader-text」クラスを持つspanタグの中に、入力したテキストが出力されている事を確認しました。
developブランチで作ったリンク設定付きのSliderブロックがこのブランチに変更してもリカバリーが発生しないことを確認しました。

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

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

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

実装者と同じ

レビュワー向け

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

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

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

[mtdkei]

ご作業いただきありがとうございます。確認項目やコードを確認しました。
2人目の方お願いいたします。

[sysbird]

@akito-38
確認しまして、動作問題ないと思います
対応ありがとうございます

deprecated の中が気になります

• さきごろバージョンアップしたので、お手数ですがバージョン番号を変更お願いしてもよいですか
• 変数の relAttribute と linkDescription が今回追加されたと思いますが、これは次回の deprecate に反映されるものかと「 次回対応おねがいします」のようなコメントで書いておくと次回の対応するメンバーが助かります
• test/e2e-tests/fixtures/blocks の中にもファイルを追加必要な気がします〜
  → (参照) https://www.notion.so/VK-Blocks-deprecated-166042e1a2e480e3989bd2d699093dbf

[sysbird]

もしかしたら 変数追加では deprecate いらないかもですので、終礼で確認するとよいかもです
→ 他のブロックも同じ対応で deprecate してるので必要なのでしょうかね？わからなくてすみません
https://discord.com/channels/1010092469863587892/1010100104046325771/1320981417546616915

[mtdkei]

@sysbird
横からですみません！おそらく宮本さんはこちらのプルリクを参考に作っていただいてるので私から返事します。

このブランチを読み込む前にすでにリンク付きのブロックがある場合、本変更後のブランチに切り替えてビルドすると、deprecateがない状態ではnoreforror、nofforrowの表示がなくなってしまうのでつけています。

• deprecate追加なし
  
• deprecate追加あり
  

[mthaichi]

このブランチを読み込む前にすでにリンク付きのブロックがある場合、本変更後のブランチに切り替えてビルドすると、deprecateがない状態ではnoreforror、nofforrowの表示がなくなってしまうのでつけています。

@mtdkei いろいろとご対応ありがとうございます！

そういった「deprecateがない状態ではnoreforror、nofforrowの表示がなくなってしまうのでつけています。」という対処法は、よろしくないかなーという感じです。
明らかにおかしな挙動に対して、よくわからないけどこうしたら動いたからそうしています。というのは後々大変なことになるのでやめたほうが良いです。
ご面倒かと思いますが、論理的になぜ動いたのか動かないかを検証してください。

とりさんがおっしゃるように、新規のattributes追加では、deprecatedを書かなくても良いと私は考えています。
ただ、前からのブロックは新規のattributesに対してはundedinedが入ります。
そのundefinedをそのままコンポーネントに渡している可能性があります。コンポーネントは undefinedがあるのでチェックボックスを表示しないということが推測されます。 これにdefault値を適用したい場合は、edit.jsで undefinedを検知してdefault 値にしてあげると良いと思います。

外してたらすみません。

あと、deprecatedを書くケースで、新しいattributesを古いブロックに適用させる方法としては、migrateという方法もあります。
migrate については以下のコードを参考にしてください。

vk-blocks-pro/src/blocks/balloon/deprecated/index.js

Lines 116 to 121 in 7ad3cdf

	migrate: (attributes) => {
	return {
	...attributes,
	balloonIconDisplay: false,
	};
	},

こちらも参考ください。
https://ja.wordpress.org/team/handbook/block-editor/reference-guides/block-api/block-deprecation/

他のブロックでも同様の対応をしていたら修正が必要かもしれません。
上記、お手数をおかけしますが、改めてご確認どうぞよろしくお願いいたします。

[mthaichi]

よくわからない動きをする時になぜそうなるか全然わからない！ 手に負えない！という場合は、遠慮なくお声がけくださいね。みなさま。

[akito-38]

@sysbird @mtdkei @mthaichi
確認ありがとうございます。
deprecateの件は、migrateで対応いたしました。
バージョン番号の変更と、test/e2e-tests/fixtures/blocks の中にもファイルを追加したので確認お願い致します。

[mtdkei]

@mthaichi
ご指摘ありがとうございます。
理解不足から、今回の対応が場当たり的になってしまっていた点は理解しました。申し訳ございませんでした。

すみませんが、正直なところ、自分では場当たり的な処理をしたつもりはなく、そもそもこのようなケースではどのように考えて対応すべきなのかを把握できていませんでした。(変数追加では deprecate は必要がない、というのは話の中で承知はしておりましたが、どこまでがその範囲なのかを理解しておらず、色々と確認するにあたり、状況によっては変数追加でも deprecate は追加するものかと思っておりました。)

今後はもう少し適切な対応ができるようにしたいと考えています。
そのため、以下の点についてアドバイスをいただけますでしょうか？

• 新規 attributes を追加した場合、既存のブロックでは undefined が発生するため、基本的に edit.js で undefined を検知して default 値を適用する、という運用で問題ないでしょうか？
• 「新規の attributes 追加では deprecated は不要」とのことですが、全てのケースで不要と考えてよいのでしょうか？例外はあるのかもしれませんが。
• 念のため確認ですが、deprecated を使わず migrate を使うべきかについてドキュメントを確認したところ、以下のケースで使用する認識で合っていますでしょうか？
  • buttonColor → btnColor のように 属性名を変更する場合
  • speaker や message のように、バラバラの attributes を content というオブジェクトに統合する場合

自分でも調べながら対応したいと思っていますので、方向性のヒントをいただけると助かります。
お手数をおかけし申し訳ありませんが、よろしくお願いいたします。

[sysbird]

@akito-38
対応ありがとうございました
動作と deprecated の内容を再度確認しました
マージしますね

今回の readme の修正で、
[ Add function ][ Icon / Slider ]
とあるので 同じ修正をスライダーに行ってると思います
スライダーの deprecated もアイコンブロックのように合わせるとよいかと思いますー

[sysbird]

修正はいったから もうおひとり確認していただいたほうが安心でしょうか？
@mthaichi か @mtdkei お願いできますか

[mthaichi]

@mtdkei 取り急ぎ返信です。またどこかで知見の共有が必要ですね。

• 新規 attributes を追加した場合、既存のブロックでは undefined が発生するため、基本的に edit.js で undefined を検知して default 値を適用する、という運用で問題ないでしょうか？

はい、問題ないと思います。

• 「新規の attributes 追加では deprecated は不要」とのことですが、全てのケースで不要と考えてよいのでしょうか？例外はあるのかもしれませんが。

不要でおそらく例外はないと私は考えています。deprecatedが必要なのはsave.jsが出力するHTMLが変更される場合です。
コード量が増えるのはできるだけ避けたいので、「心配だからdeprecatedをいれる」はお控え頂ければと。

• 念のため確認ですが、deprecated を使わず migrate を使うべきかについてドキュメントを確認したところ、以下のケースで使用する認識で合っていますでしょうか？

  • buttonColor → btnColor のように 属性名を変更する場合
  • speaker や message のように、バラバラの attributes を content というオブジェクトに統合する場合

その理解であっていると思います。

[mthaichi]

@akito-38 ご対応ありがとうございます！
問題ないと思いますのでマージしました。

[mtdkei]

@mthaichi
ご確認いただきありがとうございました。次からはそのように心がけます。