bullet-train-co / bullet_train-sortable

MIT License
0 stars 1 forks source link

enableSortable doesn't run after turbo-frame updates #4

Closed jdwyah closed 2 years ago

jdwyah commented 2 years ago

Using turbo streams, sortable does not re-initialize after a turbo update.

I'm using turbo frames and more specifically a turbo_stream.update to re-render a list page.

As a super hack I tried catching 'turbo:frame-load' and triggering 'sprinkles:update' so that the sortable hooks run. This worked, but it was pretty gross. Moreover, there is no turbo after-stream render event (and looks like there never will be https://github.com/hotwired/turbo/issues/92 ) so I can't get sortable to re-initialize then.

The reason turbo isn't going to send that event is because they want everything to move to stimulus. AFAICT that makes sense here too and everything will work if we stimul-ify https://github.com/bullet-train-co/bullet_train-sortable/blob/main/app/javascript/concerns/sortable.js

I can try to PR that in if this sounds like the right approach to you.

pascallaliberte commented 2 years ago

Here's what I would do:

  1. wrap the sortable collection inside a stimulus controller and make sure turbo_stream.update replaced the entire list. If turbo_stream.update doesn't replace the whole list, see Option B below.
  2. on connect() of that Stimulus controller wrapping the sortable collection, dispatch a sprinkles:update jQuery event.

Option B:
If turbo_stream.update doesn't replace the whole list, you could use stimulus-use.useTargetMutation to catch when an element of sortable has been replaced, and then re-dispatch a sprinkles:update jQuery event.

jdwyah commented 2 years ago

mm, thanks for taking a look. I think what you're suggesting will work. I still feel weird firing sprinkles:update but can't argue with it working :) Is there a reason that making sortable a stimulus controller itself sounds like the wrong approach to you? (Just clarifying whether you're suggesting a workaround or that this is the ideal)

pascallaliberte commented 2 years ago

@jdwyah Oh, for sure this is a workaround, and you're right that re-running sprinkles:update is icky. If you'd like to propose a proper Stimulus controller, go for it. Otherwise I might work on one.

andrewculver commented 2 years ago

@pascallaliberte Please work on one at your earliest convenience. :-)

pascallaliberte commented 2 years ago

@andrewculver On my list for sure

jdwyah commented 2 years ago

I started hacking on this, but bailed bc I wasn't sure how you'd like to deal with changing attribute names to be more stimulousy & how that would impact the generators etc. I didn't see an obvious backwards compatible solution.

andrewculver commented 2 years ago

@jdwyah Not worried about backwards compatibility on this particular thing. We can always tell people what's up and how to update (if it makes sense to change.)

pascallaliberte commented 2 years ago

I looked into it a bit. I think a lot of the selectors we see in the sprinkles version (for different handles and things) are generated by dragula on init.

For scaffolding, there’s a single line in super_scaffolding to update: from data-reorder to a data-controller and data values attributes.

So two repos for sure, but also the main starter repo will need an update, for targeting new version numbers.

pascallaliberte commented 2 years ago

This might enough of a breaking change for upping major versions?

andrewculver commented 2 years ago

@pascallaliberte I think we'll skip any major version bumps for now until we stabilize the OSS release, then we'll bump to a 1.2 series.

pascallaliberte commented 2 years ago

Got started on this today, see PR #6.

@jdwyah I think I'm going to provide an action so you can re-setup the sortable via the new Stimulus controller, and the marvellous trick of auto-destructing Stimulus controllers that Matt Swanson wrote about recently should work beautifully.

More on that next week.

pascallaliberte commented 2 years ago

@jdwyah I've updated PR #6 with a Stimulus controller that works and which automatically gets re-run after turbo-frame updates. No need for the trick mentioned above.

You'll need to follow instructions in the PR's description to enable this temporary version for testing. Whether you wholesale override the <tbody> controller by the Stimulus controller, or whether you just update its descendant elements, the sortable functionality will always be maintained (re-applied as needed).

Can you test it to see if this does the job?

jdwyah commented 2 years ago

Awesome! AFK this week, but I will try this and let you know.

jdwyah commented 2 years ago

Can confirm that https://github.com/bullet-train-co/bullet_train-sortable/pull/6 works great for me. It reapplies after a turbo update no problem.