Conversation
uidev1116
left a comment
uidev1116
left a comment
There was a problem hiding this comment.
@1000-x-t30
気になったところコメントつけさせていただきました。
ご確認よろしくお願いします。:bow:
src/core/index.js
Outdated
| const currentItem = this._getSelectedItem(); | ||
|
|
||
| const onFocus = (e) => { | ||
| if(e.keyCode === 9) { |
There was a problem hiding this comment.
@1000-x-t30
KeyboardEvent.keyCode は非推奨になってるみたいなので、KeyboardEvent.key を活用したほうがプログラムが長持ちしそうです。
https://developer.mozilla.org/ja/docs/Web/API/KeyboardEvent/keyCode
念のためIEも対応したい場合は↓とかでもいいかもです。
いかがでしょうか？
e.key === 'Tab' || e.keyCode === 9;
| class="\{classNames.smartPhotoDismiss\}" | ||
| data-action-click="hidePhoto()"> | ||
| <span class="smartphoto-sr-only">\{message.closeDialog\}</span> | ||
| </button> |
There was a problem hiding this comment.
@1000-x-t30
おそらく button タグの位置が変わったためだと思うのですが、バツボタンをクリックしても hidePhoto が実行されなくなってしまってます。！！
修正お願いします。！！
src/core/index.js
Outdated
|
|
||
| const onFocus = (e) => { | ||
| if(e.keyCode === 9) { | ||
| const smartPhoto = document.querySelector(`.${this.data.classNames.smartPhoto}`); |
There was a problem hiding this comment.
@1000-x-t30
すみません。この実装だとTabを押してフォーカスを移動させたときは良いのですが、shift + Tab でフォーカスを戻ったときに、フォーカスがモーダルの外に行ってしまいます。
以下は jQuery での例ですが、ライブラリを利用しない場合はこんな感じが良いかもです。
var focusableElements = 'a[href], area[href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), button:not([disabled]), object, embed, *[tabindex], *[contenteditable]';
var $first = $modal.find(focusableElements).filter(":visible").first();
var $last = $modal.find(focusableElements).filter(":visible").last();
$first.off('keydown.acms-dialog').on('keydown.acms-dialog', function (e) {
if ((e.which === 9 && e.shiftKey)) {
e.preventDefault();
$last.focus();
}
});
$last.off('keydown.acms-dialog').on('keydown.acms-dialog', function (e) {
if ((e.which === 9 && !e.shiftKey)) {
e.preventDefault();
$first.focus();
}
});
$first.focus();修正よろしくお願いします。！！
src/core/index.js
Outdated
|
|
||
| const smartPhoto = document.querySelector(`.${this.data.classNames.smartPhoto}`); | ||
| smartPhoto.focus(); | ||
| document.addEventListener('keydown', onFocus); | ||
| this._registerRemoveEvent(window, 'keydown', onFocus); |
There was a problem hiding this comment.
ここですが、数行下にも同じ処理があるので、if ~ else の外で1回の処理で済ませたほうが良いかと思いました。
また、そもそも addNewItem 内でやらなくてもいい気がしてます…。
ご検討よろしくお願いします。:bow:
src/core/index.js
Outdated
| this._fireEvent('open'); | ||
| }); | ||
|
|
||
| const smartPhoto = document.querySelector(`.${this.data.classNames.smartPhoto}`); |
There was a problem hiding this comment.
@1000-x-t30
以下のような処理がいくつか出てきますが、.${this.data.classNames.smartPhoto} の要素がHTML上に存在した場合に意図した挙動にならないかと思いますので、[data-id='${this.id}'] を前につけてあげたほうが良いかもです。
lite-editor のコードが参考になるので共有します。
https://github.com/appleple/lite-editor/blob/101a3b4f93a4a0198503cf84260711425e6d276a/src/core/index.js#L299
ご確認よろしくお願いします。！:bow:
There was a problem hiding this comment.
@1000-x-t30
すみません。こちら意図が正しく伝えられてなかったです。
結論としては、要素を取得する場合には this._getElementByClass メソッドを活用してくださいというところになります。
data-id="className" を取得したい要素につけるでも悪くはないのですが、不要な data-id が乱立するのでスマートではないと思いますがいかがでしょうか？
There was a problem hiding this comment.
@uidev1116
なるほど...
モーダル自身のdata-idを利用し詳細度を高くするということですね！
確かに不要なdata-idを増やさずに安全に実装できると思います、ありがとうございます🙇
src/core/index.js
Outdated
|
|
||
| _onFocus(e) { | ||
| const smartPhoto = document.querySelector(`[data-id='${this.data.classNames.smartPhoto}']`); | ||
| const dismiss = document.querySelector(`[data-id='${this.data.classNames.smartPhotoDismiss}']`); |
There was a problem hiding this comment.
@1000-x-t30
ここですが、smartPhoto 及び dismiss の要素を取得したいのではなく、モーダル内でフォーカス可能な最初と最後の要素を取得したいということだと思うので、モーダル内でフォーカス可能な最初と最後の要素を取得するという実装にしませんか？
現状の問題点は、今後、viewer.html のテンプレートを変更して、smartPhoto 及び dismiss がモーダル内でフォーカス可能な最初と最後の要素でなくなった場合に、_onFocusメソッド内も変更する必要があることです。
ご確認よろしくお願いします。:bow:
| const smartPhoto = document.querySelector(`[data-id='${this.data.classNames.smartPhoto}']`); | ||
| smartPhoto.focus(); | ||
| } | ||
| } |
There was a problem hiding this comment.
@1000-x-t30
_fireEvent メソッドはイベントを発火することが責任なので、その中で副作用となる処理を書いてしまうと保守しづらくなってしまうため修正お願いしたいです。
addNewItem メソッドで実際に open イベントを発火しているところで書くべきかと思います。
ご確認よろしくお願いします。:bow:
src/core/index.js
Outdated
| }); | ||
| } | ||
| document.addEventListener('keydown', this._onFocus.bind(this)); | ||
| this._registerRemoveEvent(window, 'keydown', this._onFocus); |
There was a problem hiding this comment.
@1000-x-t30
実際に検証できてないので申し訳ないのですが、bind してaddEventListener した関数を removeEventListener できるのでしょうか？
以下のように、そのままでは別の関数として判別されてしまい、removeEventListener できないような気がしてます。
https://log.tkyisgk.com/addeventlistener-this
ご確認よろしくお願いします。:bow:
2 participants
No description provided.