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

話題を並び替えて投票できるように #52

Merged
merged 2 commits into from
May 8, 2023

Conversation

yasu551
Copy link
Contributor

@yasu551 yasu551 commented Apr 27, 2023

No description provided.

}

onStop(e) {
console.log(e)
// TODO: DOMが確定する前にイベントが発生するため一時的にsetTimeoutしてる
setTimeout(() => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sortable:stop を sortable:sorted にすれば、
setTimeout、要らなくなるのではないかと思うのですが、
如何でしょうか?

https://github.com/Shopify/draggable/tree/master/src/Sortable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sortable:sortedだと、ドラッグ中にイベントが発生するので、ちょっと不適かな〜と思います。
(実は勉強会前にもりきよとsortedも試して、「これは違うね」となったのでした)

FHF6vR9Mrg

Copy link

@asip asip Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JunichiIto

sortable:sorted は、ソートのDOMへの反映後(DOM内でソートされた
とき)にセットした関数を呼びだすイベントですが、ドラッグ中にも
ソートがDOMに反映されるdraggableでは、(項目のドラッグ中に
ソートが発生するとセットした関数が呼ばれるのは、)確かに
不適切ですね。

sortable:stopは、(ドラッグ→)ドロップした時点でセットした関数を
呼び出すイベントで、呼び出す時点でDOMへの反映は完了して
いませんが、(ドロップした時点でセットした関数が
呼ばれる、ので、)こちらのほうが確かに適切ですね。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JunichiIto
この問題、やはりissueで上がっていたようです。
Shopify/draggable#334

そしてその解決策として、v1.0.0-beta.12でdrag:stoppedというイベントが追加されたみたいですね。
Shopify/draggable#420

ですので、sortableのバージョンを、現状の1.0.0-beta.8から1.0.0-beta.12に上げつつ、
sortable:stopイベントをdrag:stoppedイベントで置き換えれば解決しそうです!

Copy link

@asip asip Apr 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

draggable、2年半くらい前に shopify が手を引いて、引き継ぐ
メンテナが現れず、開発が(ほぼ)停止している ので、替わりのライブラリ
(Sortable etc)にしたほうがよいかもしれません。
(shopifyの方(原作者?)がごく偶にプルリクをマージしてはいます。。。
https://github.com/Shopify/draggable/commits/master)

Draggable is no longer maintained by its original authors.

https://github.com/Shopify/draggable

Sortable
https://github.com/SortableJS/Sortable
https://sortablejs.github.io/Sortable/

SortableJSであっさりリスト並び替え
https://zenn.dev/sdkfz181tiger/articles/e5979cde0238c1

HTML ドラッグ & ドロップ API を直接利用するテもありはします。

HTML ドラッグ & ドロップ API
https://developer.mozilla.org/ja/docs/Web/API/HTML_Drag_and_Drop_API
https://web.dev/i18n/ja/drag-and-drop/

参考:
Drag & Drop APIを使って要素の並べ替えを実装する
https://zenn.dev/kazuwombat/articles/f23b47f168f1d0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stimulus-sortable を使うのはどうでしょうか?
https://www.stimulus-components.com/docs/stimulus-sortable/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnattali @asip @morikiyo 情報ありがとうございます!
とりあえずissueを作りました。

#54

さくっと移行できるなら移行したいですね。。(次回のミートアップまでに)

this.positionTargets.forEach((element, index) => {
element.value = index
})
}, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@morikiyo これ、0なのに問題が解決するのはなぜなんでしょう?1000とか500とかならsetTimeoutする意味があるのかなと思うんですが。(昨日聞けば良かった)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

私もよくわかってなかったので少し調べてみました。

タイムアウトが満了するまでに予想より長い時間がかかる理由は複数あります。この節では、もっとも一般的な理由を説明します。

https://developer.mozilla.org/ja/docs/Web/API/setTimeout#%E9%81%85%E5%BB%B6%E3%81%8C%E6%8C%87%E5%AE%9A%E5%80%A4%E3%82%88%E3%82%8A%E9%95%B7%E3%81%84%E7%90%86%E7%94%B1

なので 0 でも直ぐに実行されるわけではないようです。

これは setTimeout を遅延 0 で呼び出したとしても、直ちに実行するのではなくキューに載せて、次の機会に実行するようスケジューリングされるためです。現在実行中のコードはキューにある関数を実行する前に完了しなければならず、このために実行結果の順序が想定どおりにならない場合があります。

「タイムアウトの遅延」の項の説明がわかりやすかったです。

私の想像なのですが、ブラウザが DOM 操作に忙しくて setTimeout に登録したメソッドの処理が後回しになるから、良い感じに動くのではないかと。ただ処理順が保証されるものではないはずなので、DOM の状態をチェックする処理を挟んでリトライするなどした方が安全だろうとは思います。

@@ -0,0 +1,5 @@
class AddPositionToVoteDetails < ActiveRecord::Migration[7.0]
def change
add_column :vote_details, :position, :integer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validates :position, presence: true, numericality: { only_integer: true, greater_than_or_equal_to: 0 }

みたいなバリデーションを付けた方がいいかも。

もしくはこう?

validates :position, presence: true, numericality: { in: 0.. }

https://techracho.bpsinc.jp/hachi8833/2021_03_04/104474

@morikiyo
Copy link
Contributor

morikiyo commented May 8, 2023

次回の meetup で stimulus-sortable への載せ替えをするとして、このPRはマージします。
https://www.stimulus-components.com/docs/stimulus-sortable/

@morikiyo morikiyo merged commit 5ca408e into hotwire-love:develop May 8, 2023
@asip
Copy link

asip commented May 8, 2023

@morikiyo

stimulus-sortable、ソースコードを読んだ感じ、SortableJSのonUpdate
イベントを利用していて、このonUpdateイベント、draggable の sortable:sorted
イベントと同じ挙動(のよう)なので、 stimulus-sortableを利用するなら、
(コントローラを)継承して、SortableJSの他のイベント(onEnd?)を利用する
形にすることになる、と思います。ご参考まで。

(stimulus-sortable、SortableJSのonUpdateイベント時に呼び出す
メソッド内で、ドラッグ後のindex etcを組み立てたFormDataにセット、
fetchでドラッグする要素のdata属性にセットしたURLにそのFormDataを
postして、更新する仕様になっています。
(既ににご存知かもしれませんが。。。))

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.

None yet

5 participants