-
Notifications
You must be signed in to change notification settings - Fork 433
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
Introduce turbo:{before-,}morph-{element,attribute}
events
#1097
Introduce turbo:{before-,}morph-{element,attribute}
events
#1097
Conversation
3aa2039
to
ecbc416
Compare
@afcapel @jorgemanrubia would this change help client-side plugins (like Stimulus Controllers, |
This does ease handling morphing inside of a Stimulus controller. I keep going back-and-forth on whether Turbo should just trigger an event like this, or if the generic concept of "you were just morphed" should live inside of Stimulus. Consider this: I create a 3rd party Stimulus controller that, on mutates the HTML in same way - maybe just adding a This could be handled with this event - e.g.
Also, since other libraries - HTMX, Livewire & Symfony Live Components - use morphing, it would be great to have a way that those could "hint" to Stimulus controllers that they are being morphed and then have the same behavior as when Turbo morphs. Again, I go back-and-forth on this, but part of my thinks a Thanks :) |
Thumbs up. The |
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.
I like this a lot @seanpdoyle, thank you. I've also found this need where stimulus controllers need fine-grained control over what happens when a page refresh morph them, and data-turbo-permanent
is not always an option.
This is a +1 from my side, but I'd love to hear what @afcapel thinks too.
I would like to bring @seanpdoyle's PR hotwired/stimulus#460 back to the table. This was something that was supposed to be added to Stimulus but never merged. In my opinion it is quite common to create observers in Stimulus controllers and that need will only increase with Turbo morphing. What do you think? |
I've played around with the whole morphing thing and I think there going are to be some issues with current approach, that maybe should be accounted before hand? So the whole problem starts with scripts that modify DOM on execution and append content to the end of the body or after the morphed element. Firstly, the <script> tags are not re-evaluated because of morph, which I think will need a solution, but more importantly, if for say a stimulus controller appends elements to the body on connect, like a global tooltip or select dropdown, the morph still removes them because in order of processing the nodes it firstly processes the Stimulus controller that appends the elements, e.g. to the end of the body, then it continues forward to those elements and decide that they should be removed because they do not match the new content from the new body, e.g.: before morph: <div data-controller="append"/>
<div appended-by-stimulus/> on morph: <div data-controller="append"/>
<div appended-by-stimulus/> <!-- old -->
<div appended-by-stimulus/> <!-- new -->
<!-- or -->
<div data-controller="append"/>
<div appended-by-stimulus/> <!-- new -->
<!-- depending if stimulus cleans up the element --> after morph (no matter if cleanup or not): <div data-controller="append"/> if it makes sense. This would also be the problem with script tags if they start to re-evaluate that modify the dom. From hindsight probably the stimulus controllers need to reconnect after the whole DOM was fully morphed. |
ecbc416
to
ade7649
Compare
@jorgemanrubia @afcapel I've expanded this change to include a |
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.
@seanpdoyle I'd keep the turbo:morph
focused at the standalone morphing operation. I think it's a common need wanting to tie logic to a morph refresh as a whole, and with this change you will be forced to add additional code to handle that.
I like a lot the idea of letting fine gained control over how morphing happens, but I lean towards offering specific events:
turbo:before-morph-element
turbo:morph-element
turbo:before-morph-attribute
I think that would make for a clearer API.
Thanks for working on this one 🙏!
Follow-up to [9944490][] Related to [hotwired#1083] Related to [@hotwired/turbo-railshotwired#533][] The problem --- Some client-side plugins are losing their state when elements are morphed. Without resorting to `MutationObserver` instances to determine when a node is morphed, uses of those plugins don't have the ability to prevent (without `[data-turbo-permanent]`) or respond to the morphing. The proposal --- This commit introduces a `turbo:before-morph-element` event that'll dispatch as part of the Idiomorph `beforeNodeMorphed` callback. It'll give interested parties access to the nodes before and after a morph. If that event is cancelled via `event.preventDefault()`, it'll skip the morph as if the element were marked with `[data-turbo-permanent]`. Along with `turbo:before-morph-element`, this commit also introduces a `turbo:before-morph-attribute` to correspond to the `beforeAttributeUpdated` callback that Idiomorph provides. When listeners (like an `HTMLDetailsElement`, an `HTMLDialogElement`, or a Stimulus controller) want to preserve the state of an attribute, they can cancel the `turbo:before-morph-attribute` event that corresponds with the attribute name (through `event.detail.attributeName`). Similarly, this commit adds a new `turbo:morph-element` event to be dispatched for every morphed node (via Idiomorph's `afterNodeMorphed` callback). The original implementation dispatched the event for the `<body>` element as part of `MorphRenderer`'s lifecycle. That event will still be dispatched, since `<body>` is the first element the callback will fire for. In addition to that event, each individual morphed node will dispatch one. This commit re-introduced test coverage for a Stimulus controller to demonstrate how an interested party might respond. It isn't immediately clear with that code should live, but once we iron out the details, it could be part of a `@hotwired/turbo/stimulus` package, or a `@hotwired/stimulus/turbo` package that users (or `@hotwired/turbo-rails`) could opt-into. [9944490]: hotwired@9944490 [hotwired#1083]: hotwired#1083 [@hotwired/turbo-railshotwired#533]: hotwired/turbo-rails#533
ade7649
to
abab1cf
Compare
turbo:before-morph
and re-purpose turbo:morph
turbo:{before-,}morph-{element,attribute}
events
@jorgemanrubia prior to the introduction of these events, what strategies have you been using to support Trix and Stimulus controllers in a morph-compatible way? |
Related to [#1097][] Documents the new events, namely: * `turbo:morph` * `turbo:before-morph-element` * `turbo:morph-element` * `turbo:before-morph-attribute` In addition to the new elements, mention that both `turbo:render` and `turbo:before-render` will encode either `"replace"` or `"morph"` into their respective `event.detail.renderMethod` properties. [#1097]: hotwired/turbo#1097
I've opened hotwired/turbo-site#159 to document these new events. |
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.
Thanks @seanpdoyle
We haven't found issues with Trix, I think because we aren't using a Trix instance in scenarios where a page refresh happens 🤔 . In the calendar, there is a single case where we used a stimulus value-change callback to prevent an unintended change in an attribute. It hasn't been a common/recurring issue for us. |
Just wondering if there was ever any progress on a tighter integration with morphing in stimulus? I know I'll have many cases of incompatibility as I go through my controllers and test them out, but one simple one is a button that starts off hidden and the Right now as soon as the page is morphed the button disappears again but the connect method never re-runs. It's almost as if there needs to be another version of It'd almost be better to have some kind of |
@brendon if still relevant, then you can try a bit different approach
this also works nice for updated values and connects your element back again since it gets added to the dom. |
That's an interesting approach :) I've managed to iron out all my controllers now but will try this out next time I have an issue. I found that if I only blocked updates of certain attributes (like I'm sure some smart cookie will come up with a clean way to describe these interactions in the future if turbo sticks with morphing :D |
Follow-up to 9944490
Related to #1083
Related to @hotwired/turbo-rails#533
The problem
Some client-side plugins are losing their state when elements are
morphed.
Without resorting to
MutationObserver
instances to determine when anode is morphed, uses of those plugins don't have the ability to prevent
(without
[data-turbo-permanent]
) or respond to the morphing.The proposal
This commit introduces a
turbo:before-morph-element
event that'lldispatch as part of the Idiomorph
beforeNodeMorphed
callback. It'llgive interested parties access to the nodes before and after a morph. If
that event is cancelled via
event.preventDefault()
, it'll skip themorph as if the element were marked with
[data-turbo-permanent]
.Along with
turbo:before-morph-element
, this commit also introduces aturbo:before-morph-attribute
to correspond to thebeforeAttributeUpdated
callback that Idiomorph provides. Whenlisteners (like an
HTMLDetailsElement
, anHTMLDialogElement
, or aStimulus controller) want to preserve the state of an attribute, they
can cancel the
turbo:before-morph-attribute
event that correspondswith the attribute name (through
event.detail.attributeName
).Similarly, this commit adds a new
turbo:morph-element
event to bedispatched for every morphed node (via Idiomorph's
afterNodeMorphed
callback). The original implementation dispatched the event for the
<body>
element as part ofMorphRenderer
's lifecycle. That event willstill be dispatched, since
<body>
is the first element the callbackwill fire for. In addition to that event, each individual morphed node
will dispatch one.
This commit re-introduced test coverage for a Stimulus controller to
demonstrate how an interested party might respond. It isn't immediately
clear with that code should live, but once we iron out the details, it
could be part of a
@hotwired/turbo/stimulus
package, or a@hotwired/stimulus/turbo
package that users (or@hotwired/turbo-rails
) could opt-into.